-
Notifications
You must be signed in to change notification settings - Fork 6
Adding mise.toml file generation. #1629
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
Conversation
@@ -27,6 +27,8 @@ const ( | |||
native = "native" | |||
// workflows for aws-native provider | |||
awsNative = "aws-native" | |||
// mise.toml file generation | |||
mise = "mise" |
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.
Created a mise
mixin. I think we should move to a capabilities or feature-based approach instead of base
vs native
. We should be requiring the different composable templatesets (thanks @mjeffryes for the suggestion).
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.
maybe we should name this something slightly more generic and move the devbox + devcontainer templates in here too.
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 like this idea but I expect we'll run into problems if/when we start trying to leverage mise.toml
in CI via the setup-mise action. The tool setup steps are sprinkled all over, and we don't want to run those existing setup steps in addition to this mise one.
In order for this mixin to also provide setup-wise functionality, I think we would need to essentially:
- Consolidate all the tool setup into a reusable action.
- Have the mise mixin overwrite that reusable action?
It would work but it's a little odd. Or make the existing tool setup its own mixin, and the templates can choose which one they want.
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 don't think we have to worry about the tool setup yet. Alberto just needs mise installed in ci mgmt so the test he added for the test providers passes. We can worry about integrating ci later
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.
Let's discuss offline today what the path forward should be.
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.
Also, another question we need to answer is whether we extend the Dockerfile in .devcontainer
to use Mise as well.
@@ -0,0 +1,13 @@ | |||
# WARNING: This file is autogenerated - changes will be overwritten when regenerated by https://github.com/pulumi/ci-mgmt | |||
|
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 file is missing the open overrides section that we need to implement where .ci-mgmt.yml
will inject the local overrides (e.g. kubectl
if you are developing pulumi-kubernetes
).
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 planning to add this on the next PR)
@@ -51,6 +51,9 @@ test-provider/%: bin/provider-ci $(ACTIONLINT) | |||
cp -r test-providers/$(PROVIDER_NAME) bin/test-provider | |||
cd bin/test-provider/$(PROVIDER_NAME) && git init | |||
cd bin/test-provider/$(PROVIDER_NAME) && ../../../$(ACTIONLINT) -config-file ../../../../.github/actionlint.yaml | |||
# Experimental mise flag required to support building golang binaries (e.g. pulumictl) | |||
mise settings experimental=true | |||
cd bin/test-provider/$(PROVIDER_NAME) && mise trust && mise install && mise env |
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.
Tests that what we are generating is actually valid mise that could be installed. Ideally, we could use mise cfg
or mise ls
and query for the values but would require more work.
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.
Left some comments, but I think this could ship as is once we add mise to the ci job
@@ -27,6 +27,8 @@ const ( | |||
native = "native" | |||
// workflows for aws-native provider | |||
awsNative = "aws-native" | |||
// mise.toml file generation | |||
mise = "mise" |
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.
maybe we should name this something slightly more generic and move the devbox + devcontainer templates in here too.
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.
One small hiccup to consider is our automated pu/pu scripts will bump .pulumi/version
but not this version in mise.toml
, which could be problematic (example).
We can of course update the job to also modify mise.toml
, but it's a good example of why I'd really like to decouple tooling and the machinery to update it from ci-mgmt and our workflows.
@@ -27,6 +27,8 @@ const ( | |||
native = "native" | |||
// workflows for aws-native provider | |||
awsNative = "aws-native" | |||
// mise.toml file generation | |||
mise = "mise" |
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 like this idea but I expect we'll run into problems if/when we start trying to leverage mise.toml
in CI via the setup-mise action. The tool setup steps are sprinkled all over, and we don't want to run those existing setup steps in addition to this mise one.
In order for this mixin to also provide setup-wise functionality, I think we would need to essentially:
- Consolidate all the tool setup into a reusable action.
- Have the mise mixin overwrite that reusable action?
It would work but it's a little odd. Or make the existing tool setup its own mixin, and the templates can choose which one they want.
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.
What's here looks like a good point to start at.
Minor nit-pick level comments:
- I'm not convinced that very small per-feature template directories will make things easier to manage. I'd still lean towards just placing this in the "base" directory personally, but happy to see how this goes with other features.
- If we are doing the feature-based template directory, I think this default feature should be applied before the more specific provider templates, rather than after, so specific types of providers could override their mise file.
- For bridged providers, they definitely don't use pulumictl any more, so wouldn't need this downloaded via mise. One option could be in the bridged template directory to add a different variation of the mise.toml, without pulumictl. The other option would be to replicate the work from the bridged providers intot the native providers to remove the need for pulumictl.
@@ -39,21 +41,21 @@ func getTemplateDirs(templateName string) ([]TemplateDir, error) { | |||
switch templateName { | |||
case "bridged-provider": | |||
// Any Pulumi-owned bridged provider | |||
return []TemplateDir{base, internal, bridged, internalBridged}, nil | |||
return []TemplateDir{base, internal, bridged, internalBridged, mise}, nil |
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.
In terms of layering, I think "mise" should go after "base", but before the more specific layers - in case they need to override the mise file.
return []TemplateDir{base, internal, bridged, internalBridged, mise}, nil | |
return []TemplateDir{base, mise, internal, bridged, internalBridged}, nil |
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.
Thanks for the feedback. I think the next action is to discuss pro/cons with the team since I want to make sure it aligns with our long term plan.
This is required since the latest version mise is trying to install (by default) doesn't exist.
No description provided.