feat: allow custom engine, core, files images #2454
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
h4ck3rk3y
commented
May 16, 2024
...sis_starlark_framework/test_engine/service_config_full_framework_backward_compatible_test.go
Outdated
Show resolved
Hide resolved
Deploying kurtosis with
|
| Latest commit: |
67c61a6
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://d66b2771.kurtosis-docs.pages.dev |
| Branch Preview URL: | https://gyani-other-repos.kurtosis-docs.pages.dev |
leoporoli
reviewed
May 17, 2024
| } | ||
|
|
||
| if engineAuthor != defaultEngineAuthor && engineVersion == defaultEngineVersion { | ||
| return stacktrace.NewError("Expected '%v' to be set as '%v' was set. Need to enter a custom engine veresion if a custom engine author is set", engineVersionFlagKey, engineAuthorFlagKey) |
There was a problem hiding this comment.
Suggested change
| return stacktrace.NewError("Expected '%v' to be set as '%v' was set. Need to enter a custom engine veresion if a custom engine author is set", engineVersionFlagKey, engineAuthorFlagKey) | |
| return stacktrace.NewError("Expected '%v' to be set as '%v' was set. Need to enter a custom engine version if a custom engine author is set", engineVersionFlagKey, engineAuthorFlagKey) |
leoporoli
reviewed
May 17, 2024
| } | ||
|
|
||
| if engineAuthor != defaultEngineAuthor && engineVersion == defaultEngineVersion { | ||
| return stacktrace.NewError("Expected '%v' to be set as '%v' was set. Need to enter a custom engine veresion if a custom engine author is set", engineVersionFlagKey, engineAuthorFlagKey) |
There was a problem hiding this comment.
Suggested change
| return stacktrace.NewError("Expected '%v' to be set as '%v' was set. Need to enter a custom engine veresion if a custom engine author is set", engineVersionFlagKey, engineAuthorFlagKey) | |
| return stacktrace.NewError("Expected '%v' to be set as '%v' was set. Need to enter a custom engine version if a custom engine author is set", engineVersionFlagKey, engineAuthorFlagKey) |
leoporoli
reviewed
May 17, 2024
| @@ -18,6 +18,7 @@ import ( | |||
|
|
|||
| const ( | |||
| defaultEngineVersion = "" | |||
| defaultEngineAuthor = "kurtosistech" | |||
There was a problem hiding this comment.
can we move this to this package: cli/cli/defaults/defaults.go ?
There was a problem hiding this comment.
now I wonder if this value shouldn't be in the launcher and an empty string makes the launcher use the default value. WDYT?
leoporoli
reviewed
May 17, 2024
| @@ -108,6 +110,7 @@ func NewEngineServerArgs( | |||
| grpcListenPortNum uint16, | |||
| logLevelStr string, | |||
| imageVersionTag string, | |||
| imageRepository string, | |||
There was a problem hiding this comment.
why do you use imageRepository here instead of imageAuthor ?
leoporoli
reviewed
May 17, 2024
| @@ -14,7 +14,8 @@ This command will do nothing if the Kurtosis engine is already running. | |||
| You may optionally pass in the following flags with this command: | |||
| * `--log-level`: The level that the started engine should log at. Options include: `panic`, `fatal`, `error`, `warning`, `info`, `debug`, or `trace`. The engine logs at the `info` level by default. | |||
| * `--version`: The version (Docker tag) of the Kurtosis engine that should be started. If not set, the engine will start up with the default version. | |||
| * `--author`: "The author (Docker username) of the Kurtosis engine that should be started (blank will start the kurtosistech version). The same author and version are used for the API Container & Files Artifact Expander. | |||
There was a problem hiding this comment.
Does the same version also be used? or is it only the author? What happens if the user sets a custom version with the --api-container-version in the kurtosis enclave add command?
leoporoli
reviewed
May 17, 2024
| @@ -414,6 +426,7 @@ func (manager *EnclaveManager) RestartAllEnclaveAPIContainers(ctx context.Contex | |||
|
|
|||
| // this way we are going to use always the same engine's version | |||
| useDefaultApiContainerVersionTag := "" | |||
| useDefaultImageAuthor := "kurtosistech" | |||
There was a problem hiding this comment.
can we use the defaultImageAuthor constant instead?
leoporoli
approved these changes
May 17, 2024
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 doesn't work at the moment, engine starts fine core doesn't
the bug is that the apiContainerVersion tag is empty