-
Notifications
You must be signed in to change notification settings - Fork 10
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
parameterize helm chart version when spawning daemons #304
Conversation
currently without being able to pin helm chart versions deployed by the spawner, new daemons will use latest versions using latest versions is discouraged as it potentitally introduces unwanted changes this feature introduces the ability to specify the helm chart version and thereby pin helm chart versions deployed by the controller
todo
|
I'm also going to add fields for helm chart repo and lotus image tag |
adds lotus docker image options allows changing helm repo url for lotus-bundle chart update html and js
I will not be adding any tests in this PR, as it will require some code restructuring outside the scope of this |
most fields already populated from json. remove default lotus image tag from form
98de37a
to
d49c364
Compare
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.
LGTM with Will's suggested commits.
For the "sidecar" image, where is the Dockerfile for that, should we want to go ahead and publish our own at different Lotus version tags? cc: @coryschwartz
add placeholder valus Co-authored-by: Will <will.scott@protocol.ai>
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.
This looks great.
} | ||
if d.HelmChartVersion != "" { | ||
client.ChartPathOptions.Version = d.HelmChartVersion | ||
} | ||
chartPath, err := client.ChartPathOptions.LocateChart("lotus-bundle", |
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 wonder if the actual chart should be configurable also? -- although there is no garuantee that it would work of course unless it somehow had the same values file.
I'm thinking if we end up making some snowflake bots that uses a different chart, it might have the same url, but a different chart name. That hopefully never happens, but that seems as likely as having the same chart name with a different repo url. I don't have a strong opinion about this, just thinking if one is configurable the other may as well be.
controller/static/index.html
Outdated
</div> | ||
<div id="botLotusDockerRepo"> | ||
<label for="newBotLotusDockerRepo" class="form-label">Lotus Docker Image (optional)</label> | ||
<input class="form-control" type="text" id="newBotLotusDockerRepo" value="" placeholder="coryschwartz/lotus"> |
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.
Ultimately, it'd be great to move this to filecoin/lotus. coryschwartz/lotus:sidecar is based on this filecoin-project/lotus#6544 -- just lotus with the init to download latest stateroots built in. With that merged, we can have nightly and stable releases.
currently without being able to pin helm chart versions deployed by the spawner, new daemons will use latest versions
using latest versions is discouraged as it potentitally introduces unwanted changes
this feature introduces the ability to specify the helm chart version and thereby pin helm chart versions deployed by the controller