Skip to content

Add check for version guards in erb templates #10297

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

Merged
merged 1 commit into from
Apr 2, 2024
Merged

Conversation

roaks3
Copy link
Contributor

@roaks3 roaks3 commented Mar 26, 2024

We sometimes break downstream providers when invalid version guards are introduced in .erb files. This happens because we have 3+ versions to consider, with each being a superset of the next:

"private" or "alpha" -> "beta" -> "ga"

The typical version guard would be <% unless version == 'ga' -%>, which means the feature should not be available in the GA provider, but should be in Beta and Private/Alpha. Sometimes users will assume <% if version == 'beta' -%> is equivalent, but it means the feature will NOT be available in Private/Alpha, which is almost always wrong.

In short, our features should never target only beta, because it would break the assumption of Private/Alpha being a superset. This logic also works in reverse (where we wouldn't want any GA-specific functionality to also be present in Private/Alpha).

Release Note Template for Downstream PRs (will be copied)


@roaks3
Copy link
Contributor Author

roaks3 commented Mar 26, 2024

@c2thorn this isn't complete yet, but I was wondering if you could give it a quick look to make sure the approach seems reasonable, and will not be too much work to change later (assuming we ultimately stop using erb templates). I'm planning to have it run with the other mm checks against all erb files that have changed in the submitted diff.

@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR hasn't generated any diffs, but I'll let you know if a future commit does.

@c2thorn
Copy link
Member

c2thorn commented Mar 27, 2024

This would run as a separate check on PR's?

Yeah I think this is a good approach. Seems easy enough to swap out the identifying text when we swap to Go .tmpl templates. It will eventually look something along the lines of {{- if eq $.MinVersion "ga" }}

@roaks3
Copy link
Contributor Author

roaks3 commented Mar 27, 2024

Great, thanks!

Yea, it would be a separate PR check, something to the effect of git diff --name-only ../../*.erb | go run main.go

@roaks3 roaks3 force-pushed the add-version-guard-check branch from 408ceba to cf84171 Compare April 1, 2024 16:57
@roaks3 roaks3 marked this pull request as ready for review April 1, 2024 19:10
@roaks3
Copy link
Contributor Author

roaks3 commented Apr 1, 2024

Note: this is ready for review now. The plan is to run this command in a check as a separate PR.

@roaks3 roaks3 force-pushed the add-version-guard-check branch from cf84171 to 2684de1 Compare April 1, 2024 19:12
@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR hasn't generated any diffs, but I'll let you know if a future commit does.

@roaks3 roaks3 requested a review from c2thorn April 1, 2024 19:37
@roaks3 roaks3 merged commit 17740c9 into main Apr 2, 2024
9 checks passed
@roaks3 roaks3 deleted the add-version-guard-check branch April 2, 2024 13:35
rafalmaczewski pushed a commit to rafalmaczewski/magic-modules that referenced this pull request Apr 3, 2024
cmfeng pushed a commit to cmfeng/cmfeng-magic-modules that referenced this pull request Apr 5, 2024
hao-nan-li pushed a commit to hao-nan-li/magic-modules that referenced this pull request Apr 9, 2024
balanaguharsha pushed a commit to balanaguharsha/magic-modules that referenced this pull request Apr 19, 2024
balanaguharsha pushed a commit to balanaguharsha/magic-modules that referenced this pull request May 2, 2024
balanaguharsha pushed a commit to balanaguharsha/magic-modules that referenced this pull request May 2, 2024
BBBmau pushed a commit to BBBmau/magic-modules that referenced this pull request May 8, 2024
pawelJas pushed a commit to pawelJas/magic-modules that referenced this pull request May 16, 2024
pengq-google pushed a commit to pengq-google/magic-modules that referenced this pull request May 21, 2024
Cheriit pushed a commit to Cheriit/magic-modules that referenced this pull request Jun 4, 2024
pcostell pushed a commit to pcostell/magic-modules that referenced this pull request Jul 16, 2024
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