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

filesystem.go: add warning when path matches device #1412

Merged
merged 2 commits into from
Oct 25, 2022
Merged

filesystem.go: add warning when path matches device #1412

merged 2 commits into from
Oct 25, 2022

Conversation

prestist
Copy link
Collaborator

#1397

Added simple check at filesystem validation to ensure that we warn when the Path matches the Device.

Naming is hard, could use some input around Err name, and function naming. "ValidatePath" was taken so I tried to make do.

@prestist prestist requested a review from bgilbert June 29, 2022 20:01
@prestist
Copy link
Collaborator Author

Ready for review!

@prestist prestist force-pushed the path-validation branch 3 times, most recently from 81be3a5 to 4ea31d3 Compare July 6, 2022 21:09
bgilbert
bgilbert reviewed Jul 7, 2022
View reviewed changes
Copy link
Contributor

@bgilbert bgilbert left a comment

Choose a reason for hiding this comment

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

FWIW, I don't see much point to the refactor in the second commit. It doesn't meaningfully simplify the code (e.g. by separating out local variables or clarifying what each section does), and does add extra structure to navigate. Or put the other way, IMO it's easy enough to look at each iterand in the original function and see what the code is trying to do. But if you think the change meaningfully helps, I'm okay with it.

config/shared/errors/errors.go Outdated Show resolved Hide resolved
config/v3_0/types/storage.go Outdated Show resolved Hide resolved
config/v3_0/types/storage_test.go Outdated Show resolved Hide resolved
config/v3_0/types/storage_test.go Show resolved Hide resolved
config/v3_2/types/storage.go Outdated Show resolved Hide resolved
config/v3_0/types/storage.go Outdated Show resolved Hide resolved
@prestist prestist force-pushed the path-validation branch 4 times, most recently from cd4f10d to 34b8c6e Compare July 7, 2022 15:33
@prestist
Copy link
Collaborator Author

prestist commented Jul 7, 2022

FWIW, I don't see much point to the refactor in the second commit. It doesn't meaningfully simplify the code (e.g. by separating out local variables or clarifying what each section does), and does add extra structure to navigate. Or put the other way, IMO it's easy enough to look at each iterand in the original function and see what the code is trying to do. But if you think the change meaningfully helps, I'm okay with it.

I think it does. From my perspective, naming the responsibility's of the many loops gives a high level overview of what is being "validated()". Additionally it allows for each set of validation to be individually tested. (is not currently lol but can be). So while its not dramatic I think it does add value at a glance.

@bgilbert
Copy link
Contributor

bgilbert commented Jul 7, 2022

Remember to update the release notes, too.

@prestist prestist force-pushed the path-validation branch 5 times, most recently from 09028b3 to 5d1a0d1 Compare July 8, 2022 19:14
bgilbert
bgilbert reviewed Jul 10, 2022
View reviewed changes
config/v3_0/types/storage_test.go Outdated Show resolved Hide resolved
config/v3_0/types/storage.go Outdated Show resolved Hide resolved
config/v3_0/types/storage.go Outdated Show resolved Hide resolved
config/v3_0/types/storage.go Outdated Show resolved Hide resolved
config/v3_0/types/storage_test.go Outdated Show resolved Hide resolved
config/shared/errors/errors.go Outdated Show resolved Hide resolved
config/shared/errors/errors.go Outdated Show resolved Hide resolved
docs/release-notes.md Outdated Show resolved Hide resolved
@prestist
Copy link
Collaborator Author

Updated!

bgilbert
bgilbert reviewed Aug 8, 2022
View reviewed changes
config/v3_0/types/storage.go Outdated Show resolved Hide resolved
config/v3_0/types/storage.go Outdated Show resolved Hide resolved
config/v3_0/types/storage_test.go Outdated Show resolved Hide resolved
config/v3_0/types/storage_test.go Outdated Show resolved Hide resolved
config/v3_0/types/storage_test.go Show resolved Hide resolved
docs/release-notes.md Outdated Show resolved Hide resolved
docs/release-notes.md Outdated Show resolved Hide resolved
@bgilbert
Copy link
Contributor

bgilbert commented Aug 8, 2022

Also: the commit message says what the change does, but not why. The former is good but not as important, since folks can read the diff if necessary. The latter might not be clear from the diff alone.

Also also, it'd be good for the commit message to link to the issue (with the Fixes or Closes keywords so GitHub will autoclose it).

@prestist prestist force-pushed the path-validation branch 4 times, most recently from 1bc9b12 to 108b176 Compare August 17, 2022 21:17
@prestist
Copy link
Collaborator Author

prestist commented Aug 17, 2022 •
edited
Loading

Also: the commit message says what the change does, but not why. The former is good but not as important, since folks can read the diff if necessary. The latter might not be clear from the diff alone.

Great point, took a swing let me know.

Also also, it'd be good for the commit message to link to the issue (with the Fixes or Closes keywords so GitHub will autoclose it).

Absolutely thank you

@prestist prestist force-pushed the path-validation branch 2 times, most recently from 995b4a1 to fe6d7ab Compare August 30, 2022 22:02
@prestist prestist changed the title filesystem.go: add warning when path matches device filesystem.go: add warning when path matches device WIP Aug 30, 2022
@prestist prestist force-pushed the path-validation branch 3 times, most recently from c6025cd to 4ef60e5 Compare September 30, 2022 16:15
@prestist prestist changed the title filesystem.go: add warning when path matches device WIP filesystem.go: add warning when path matches device Sep 30, 2022
@prestist
Copy link
Collaborator Author

prestist commented Oct 3, 2022 •
edited
Loading

I did get this updated, I think it should be ready to rock and roll. @bgilbert

@bgilbert bgilbert closed this Oct 4, 2022
@bgilbert bgilbert reopened this Oct 4, 2022
@prestist prestist force-pushed the path-validation branch 2 times, most recently from 61344f6 to 8bdefec Compare October 4, 2022 19:11
bgilbert
bgilbert reviewed Oct 23, 2022
View reviewed changes
Copy link
Contributor

@bgilbert bgilbert 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 generally; one issue and a couple nits.

The second paragraph of the commit message could probably use some cleanup, since it isn't super-clear.

config/v3_0/types/storage_test.go Show resolved Hide resolved
config/v3_1/types/storage_test.go Outdated Show resolved Hide resolved
config/v3_1/types/storage_test.go Outdated Show resolved Hide resolved
@prestist
Copy link
Collaborator Author

Ready to go !

@prestist
storage.go: add filesystems validation warnings
a8d088f 
Fixes coreos#1397, a common configuration error is to create a filesystem over
a whole-disk device rather than a partition. Add a warning for a
filesystem device matching the disk device.

Additionally, another configuration error was identified for a way to
recreate the filesystem on every provision, while the configuration to
do so is valid, it might be done unitionally. Add a warning when
wipeTable is true but wipeFilesystem is false.
@prestist
storage.go: refactor validate()
b099378 
Separate validate() into more granular subfunctions which describe the unit
of work they are performing to improve readability and testability.
@prestist prestist merged commit bc00008 into coreos:main Oct 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bgilbert bgilbert bgilbert 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.
2 participants
@prestist @bgilbert