Skip to content
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

Typed provider configuration #1126

Merged
merged 5 commits into from
Nov 15, 2024
Merged

Typed provider configuration #1126

merged 5 commits into from
Nov 15, 2024

Conversation

blampe
Copy link
Contributor

@blampe blampe commented Nov 5, 2024

This adds strong typing around our CI configuration options.

Currently our config is just a bag of values, which has led to repos specifying options which have no effect on CI (e.g. parallel:, makeTemplate: etc.).

This adds types for all of our available options and will fail if ci-mgmt.yml specifies an unrecognized option. For backwards compatibility we continue to allow existing no-op params in order to not break existing providers.

@@ -5,7 +5,6 @@ major-version: 0
providerDefaultBranch: main
upstreamProviderOrg: vancluever
publishRegistry: false
enableAutoRelease: false
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only used by acme and not referenced in templates -- safe to remove.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, this was added but then refactored away in the same PR it appears with just this left-over.

@danielrbradley
Copy link
Member

Is there any option to conbine the defaults into the parser too so we don't have to maintain the defaults file separately?

@blampe
Copy link
Contributor Author

blampe commented Nov 5, 2024

Is there any option to conbine the defaults into the parser too so we don't have to maintain the defaults file separately?

Sure! I kept it around since this is already a pretty big change but that's very doable.

makeTemplate: bridged
template: bridged-provider
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean we need to roll out a change to every provider to update this field?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or is this a mis-typed field in the config that had no effect?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or is this a mis-typed field in the config that had no effect?

Yep! makeTemplate has no effect, so these all default to using the bridge template anyway. We'll continue to allow them to specify makeTemplate, and it'll continue to be a no-op.

Copy link
Member

@danielrbradley danielrbradley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good. Nice to have some real types involved which should make it easier to keep track of valid fields.

Would be nice to not have the documentation duplicated between the types and the defaults at some point, but that could be a follow-up task.

We'll also need a roll-out plan for fixing any bad fields in config files.

@blampe
Copy link
Contributor Author

blampe commented Nov 15, 2024

Would be nice to not have the documentation duplicated between the types and the defaults at some point, but that could be a follow-up task.

Agree and created #1146

We'll also need a roll-out plan for fixing any bad fields in config files.

We should get build failure issues for any providers specifying some bad fields not allow-listed here, and I can fix those as needed. Otherwise cleaning these up isn't urgent (arguably not worth doing). IMO it's more important that the boilerplate repos reflect the correct usage and I think you already took care of that.

@blampe blampe added this pull request to the merge queue Nov 15, 2024
Merged via the queue into master with commit a599655 Nov 15, 2024
6 checks passed
@blampe blampe deleted the blampe/typed-config branch November 15, 2024 21:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants