Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Added logging for changed overlayfs dirs #286

Merged
merged 2 commits into from
Jun 10, 2022
Merged

Added logging for changed overlayfs dirs #286

merged 2 commits into from
Jun 10, 2022

Conversation

chofnar
Copy link
Contributor

@chofnar chofnar commented May 5, 2022 •
edited
Loading

This PR implements the suggestions from issue #159.
Without --debug, it will print the first 2 changed dirs:

cache miss because content of 3 overlay dirs changed:
/home/chofnar/stacker/f2
/home/chofnar/stacker/f3
and 1 others. use --debug for complete output

With --debug, it will print all modifications:

cache miss because content of 3 overlay dirs changed:
/home/chofnar/stacker/f2
/home/chofnar/stacker/f3
/home/chofnar/stacker/f4

Newest change also lists the files that changed:
With --debug

cache miss because content of 4 overlay dirs changed:
Changed overlay_dir: /home/piggie/stackeryamls/od1
Contents that changed:
/home/piggie/stackeryamls/od1/subdir/subdirfile.txt
/home/piggie/stackeryamls/od1/file1.txt
Changed overlay_dir: /home/piggie/stackeryamls/od2
Contents that changed:
/home/piggie/stackeryamls/od2/file2.txt
Changed overlay_dir: /home/piggie/stackeryamls/od3
Contents that changed:
/home/piggie/stackeryamls/od3/file3.txt
Changed overlay_dir: /home/piggie/stackeryamls/od4
Contents that changed:
/home/piggie/stackeryamls/od4/file4.txt

Without --debug

cache miss because content of 4 overlay dirs changed:
Changed overlay_dir: /home/piggie/stackeryamls/od1
Contents that changed:
/home/piggie/stackeryamls/od1/file1.txt
/home/piggie/stackeryamls/od1/subdir/subdirfile.txt
Changed overlay_dir: /home/piggie/stackeryamls/od2
Contents that changed:
/home/piggie/stackeryamls/od2/file2.txt
and 2 other overlay_dirs. use --debug for complete output

Signed-off-by: Catalin Hofnar [email protected]

@chofnar chofnar marked this pull request as ready for review May 5, 2022 12:34
@chofnar chofnar force-pushed the logDiffDirs branch 2 times, most recently from 800df63 to ab831fb Compare May 5, 2022 14:58
@chofnar chofnar marked this pull request as draft May 5, 2022 16:16
@chofnar chofnar force-pushed the logDiffDirs branch 3 times, most recently from a90219f to eeed03a Compare May 6, 2022 12:56
@chofnar chofnar marked this pull request as ready for review May 6, 2022 13:26
rchincha
rchincha reviewed May 9, 2022
View reviewed changes
cache.go Outdated
} else {
for index, dir := range changedOverlayDirs {
log.Infof(dir)
if index == 1 && len(changedOverlayDirs) > 2 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move the "if c.config.Debug" check here, since you are doing log.Infof(dir) in both cases

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modified as requested

@chofnar chofnar force-pushed the logDiffDirs branch 2 times, most recently from 942d146 to 04e352c Compare May 10, 2022 08:51
@chofnar
Added logging for changed overlayfs dirs
797cd3e 
Adapted test to account for new output format
Added bats helper execution branch for stacker without --debug (needed by new formatting)

Signed-off-by: Catalin Hofnar <[email protected]>
@eusebiu-constantin-petu-dbk
Copy link
Contributor

LGTM

1 similar comment
@eusebiu-constantin-petu-dbk
Copy link
Contributor

LGTM

andaaron
andaaron approved these changes Jun 7, 2022
View reviewed changes
Copy link
Contributor

@andaaron andaaron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change is good, but I think the challenge was to also show what content of the overlay dir was changed (except we don't store that kind of information as far as I see). @raharper was that the ask?

Maybe it would make sense to enhance the caching logic to cache the hash per file instead of the entire folder?

@chofnar
Copy link
Contributor Author

chofnar commented Jun 8, 2022

Change is good, but I think the challenge was to also show what content of the overlay dir was changed (except we don't store that kind of information as far as I see). @raharper was that the ask?

Maybe it would make sense to enhance the caching logic to cache the hash per file instead of the entire folder?

The logic already supported something like that, with some small adaptations. Please check the new description for the updated output

@chofnar chofnar requested a review from andaaron June 8, 2022 13:59
andaaron
andaaron approved these changes Jun 9, 2022
View reviewed changes
Copy link
Contributor

@andaaron andaaron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.
Though I think we don't really need the log messages:

Changed overlay_dir: <folder_path>
Contents that changed:`

The user can figure it out on his own based on the files shown.

@andaaron andaaron merged commit 4213595 into project-stacker:master Jun 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rchincha rchincha rchincha left review comments

@andaaron andaaron andaaron approved these changes

@hallyn hallyn hallyn approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@chofnar @eusebiu-constantin-petu-dbk @hallyn @andaaron @rchincha