Clarify the error message if the release is invalid #1100
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The idea of splitting ResolveContainerAndImageNames into two public functions [1] didn't turn out to be so useful [2]. Splitting things even further might make it worse. A better approach might be to (re-)unify the code further. This is the first step towards that. This reverts the following commits: * 5c8ad7a * 02f45fd * 8b6418d ... but retains the test cases that were not tied to the changes in those commits. [1] Commit fd75608 containers#828 containers#838 [2] containers#977 containers#937 containers#1080
The idea of splitting ResolveContainerAndImageNames into two public functions [1] didn't turn out to be so useful [2]. It pushed the burden on the callers, who needed to carefully call the split functions in the right order, because the container, distro, image and release values are very tightly related. This opens the door for mistakes. A better approach would be to restore ResolveContainerAndImageNames as the single public API. If necessary it could be internally split into smaller private functions. It would keep things simple for the callers. Note that this commit doesn't include the private split. If necessary, it can be done in future. This reverts commit fd75608. [1] Commit fd75608 containers#828 containers#838 [2] containers#977 containers#937 containers#1080
The terms 'default' and 'fallback' are used to mean very specific things in this context. The 'default' values are those that are used when the 'create', 'enter' and 'run' commands were used without any option. These values are picked to match the host operating system. However, if there's no supported Toolbox image matching the host operating system, and no options were provided to the 'create', 'enter' and 'run' commands, then the 'fallback' values are used as a last resort. Consistently using this terminology leads to a clear mental model and makes the code easier to read. This rough arrangement of the code was already being used for 'release', and has now been been extended to 'container name prefix' and 'distro'. The suffix for the 'fallback' values was simplified to 'Fallback', instead of 'DefaultFallback'. containers#937 containers#1080
Currently, if an invalid or unsupported string is specified as the distro on the command line or in the configuration file, then it would silently fallback to Fedora. This shouldn't happen. It should only fallback to Fedora when no distro was specified and there's no supported Toolbox image matching the host operating system. If a distro was explicitly specified then it should either be supported or it should error out. The test cases were resurrected from commit 8b6418d. containers#937 containers#1080
This will make the subsequent commit easier to read. containers#937 containers#1100
Currently, if --release has an invalid argument, the error message doesn't give any hints as to what's an acceptable value. This can be confusing. eg., is 36 a valid argument for Fedora? Or is it f36? Or is it F36? Is 'rawhide' accepted? containers#937 containers#1100
debarshiray
force-pushed
the
wip/rishi/clarify-parse-release-error
branch
from
7f901a5 to
b5474bf
Compare
|
Build succeeded. ✔️ unit-test SUCCESS in 7m 16s |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This builds on top of #1080 It intends to fix the third part of #937 by clarifying the error message if
--releasehas an invalid argument.