lf-edge
Make parsing log filenames future-proof
Main commit is a8725c77d437b9aa412848f6e6b359d8569e26cc. Instead of relying on a certain structure of log filenames, we should just use regexp to look for timestamps or UUIDs - this way the filename structure can change in the future, but it will still be backwards compatible with our filename parsing logic.
This PR contains the changes for pillar. In a later PR those changes will also be applied to newlog and edge-view.
the yetus warning can probably be ignored since we do the same in other files that import gomega
yetus: pkg/pillar/types/newlogtypes_test.go#L10 revive: should not use dot imports https://revive.run/r#dot-imports
the yetus warning can probably be ignored since we do the same in other files that import gomega
yetus: pkg/pillar/types/newlogtypes_test.go#L10 revive: should not use dot imports https://revive.run/r#dot-imports
Actually not in all places, like here gomega it's not dot imported: https://github.com/lf-edge/eve/blob/master/pkg/pillar/types/patchenvelopestypes_test.go#L10
I would avoid as much as we can to introduce new Yetus errors and in this case it's really not hard to fix it...
@europaul , LGTM and I liked the flexibility added by using regex. However, I would fix the Yetus error in order to avoid YAYEE (Yet Another Yetus Error on EVE :smiley: )
I think we must address the same functions in the EdgeView package; otherwise, the next time we upgrade the Pillar package used by EdgeView, it will be broken.
The new functions are (mostly) backward compatible with the old ones. The only problem that I see is with logs that get created before the devices sets the correct time (probably through NTP), so the initial log files look like dev.log.123.gz instead of dev.log.1731491904032.gz. The new function will not pick up a timestamp from them since it doesn't fit into \b\d{13,16}\b regexp.
As far as I see, we still use this func in another package, edgeveiew:
@OhmSpectator yes, I will change them once this is merged and I can update the pillar dependency.
I think we must address the same functions in the EdgeView package; otherwise, the next time we upgrade the Pillar package used by EdgeView, it will be broken.
The new functions are (mostly) backward compatible with the old ones. The only problem that I see is with logs that get created before the devices sets the correct time (probably through NTP), so the initial log files look like
dev.log.123.gzinstead ofdev.log.1731491904032.gz. The new function will not pick up a timestamp from them since it doesn't fit into\b\d{13,16}\bregexp.
@europaul , can we affirm that the timestamp will always precede the file extension, like this <any file name>.<TIMESTAMP>.<extension (.gz, etc)> ?
If so, the following regex could be used \b(\d{3,16}).\w{2,3}, then the first occurrence will be the timestamp and it will cover the dev.log.123.gz case. If you want to be more picky with the timestamps, another options is to use this regex: \b(\d{13,16}|\d{3}).\w{2,3}, which will cover only valid timestamps and the case for 3 digits....
As far as I see, we still use this func in another package, edgeveiew:
@OhmSpectator yes, I will change them once this is merged and I can update the pillar dependency.
So, we are expecting a new PR very after this one is merged... Have you tested the changes in edge view somehow?
If so, the following regex could be used \b(\d{3,16}).\w{2,3}, then the first occurrence will be the timestamp and it will cover the dev.log.123.gz case. If you want to be more picky with the timestamps, another options is to use this regex: \b(\d{13,16}|\d{3}).\w{2,3}, which will cover only valid timestamps and the case for 3 digits....
it's actually not just 3 digits - the timestamp can be anything between 0 and now microseconds.
As far as I see, we still use this func in another package, edgeveiew:
@OhmSpectator yes, I will change them once this is merged and I can update the pillar dependency.
So, we are expecting a new PR very after this one is merged... Have you tested the changes in edge view somehow?
ok, let's postpone this merge until we can execute it really in sync with the PR for edge-view and newlog. I'll convert it to draft for now
The only problem that I see is with logs that get created before the devices sets the correct time (probably through NTP), so the initial log files look like dev.log.123.gz instead of dev.log.1731491904032.gz. The new function will not pick up a timestamp from them since it doesn't fit into \b\d{13,16}\b regexp.
I fixed this problem, by using the first digits-only part that we find between dots as a timestamp
So, we are expecting a new PR very after this one is merged... Have you tested the changes in edge view somehow?
@OhmSpectator should I prepare another PR with changes to edge-view and newlog that would be merged right after this one before we merge this one?
@OhmSpectator should I prepare another PR with changes to edge-view and newlog that would be merged right after this one before we merge this one?
I don’t actually remember how we update Pillar as a library. Do we first need to get some kind of hash and add it to the go.mod file of another component? If it’s possible to do within the same PR (I’m not sure), I would prefer to do it that way. I always ask @christoph-zededa or @deitch when dealing with such things.
Do we first need to get some kind of hash and add it to the go.mod file of another component?
Yes, that's how we usually do it, we need to first publish pillar before we can update it as a dependency in other modules. Afaik, it's not possible to do both in one PR.