-
Notifications
You must be signed in to change notification settings - Fork 7
Improve the BuildMe example #102
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.
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
base: main
Are you sure you want to change the base?
Conversation
cmoulliard
commented
Apr 23, 2025
- Improve the BuildMe example
- Verify if the mandatory env vars are well declared
- Add a new env var to support to enable/disable: USE_DAEMON
- Bump the version to use now: 0.0.14
Signed-off-by: cmoulliard <[email protected]>
Signed-off-by: cmoulliard <[email protected]>
Signed-off-by: cmoulliard <[email protected]>
Signed-off-by: cmoulliard <[email protected]>
Signed-off-by: cmoulliard <[email protected]>
Signed-off-by: cmoulliard <[email protected]>
Signed-off-by: cmoulliard <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just minor comments.. =)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment feels misleading here... following the code, USE_DAEMON
is essentially passed to .withUseDaemon(...)
When true, the daemon socket is mounted into the phase containers so it may be used directly by the lifecycle. This is the original mode for the snowdrop library and is thus the default. USE_DAEMON=true does not mean the lifecycle is accessing a registry directly, it means it will use the mounted docker socket.
Conversely, when USE_DAEMON=false, the lifecycle will talk directly to the target registry, (and indeed, it has to, because we do not pass it the docker socket, so it has no other way to reach the registry). One upside of USE_DAEMON=false is that the image is built directly in the destination registry, and not in the local docker daemon, so there is no need to 'publish' the generated image afterwards. (On the flipside, if you want to use the image afterwards, you'll also need to pull it to your daemon from the registry!).
The comment implies that setting USE_DAEMON to true will allow lifecycle to directly access the registry, which is the inverse of what will actually happen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will review then the text of the README to mention that what you explain here but without too much words ;-)
String PROJECT_PATH = Optional.ofNullable(System.getenv("PROJECT_PATH")) | ||
.orElseThrow(() -> new IllegalStateException("Missing env var: PROJECT_PATH")); | ||
String USE_DAEMON = Optional.ofNullable(System.getenv("USE_DAEMON")) | ||
.orElse("false"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
default for snowdrop lib is useDaemon==true (to provide consistency for the original experience using the library).. it might be nice to maintain that default here, so if USE_DAEMON is not declared, people get the default library behavior..
Alternatively, you could make the invocation of .withUseDaemon(..)
optional if the env var is omitted.. tho that will probably look a little less clean from a code perspective, because skipping a setter on the dsl is a little tricky.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking that the default was false
. Will then change the default value to true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if the logic from lines 36 to 46 (the creation and addition of an authConfig to the list), makes sense.. I know it's not part of this PR, but looking at the file during the review, I notice it's checking REGISTRY_ADDRESS, but configuring 'registry' the var that will set the address from REGISTRY_SERVER.. I suspect these two var names are supposed to match.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with you that alignment is required between the README.md file and the code.
We only need 3 env variables
String registry = System.getenv("REGISTRY_SERVER");
String username = System.getenv("REGISTRY_USER");
String password = System.getenv("REGISTRY_PASS");
I will fix that