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

chore: add codegen CI generated code check #714

Merged
merged 2 commits into from
May 12, 2024

Conversation

syall
Copy link
Contributor

@syall syall commented May 7, 2024

Issue #

The Weather SDK codegen is out of sync, and there is no mechanism to check it.

Description of changes

Add codegen CI generated code check.

Scope

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@syall syall force-pushed the ensure-weather-sdk-codegen-current-ci branch 3 times, most recently from 2bd70b0 to bcd333c Compare May 7, 2024 02:55
@syall syall marked this pull request as ready for review May 7, 2024 03:02
@syall syall force-pushed the ensure-weather-sdk-codegen-current-ci branch 3 times, most recently from 0e5a1f1 to 9de0a57 Compare May 9, 2024 17:10
@syall syall changed the title feat: add codegen CI check feat: add check to see if generated code is current May 9, 2024
@syall syall force-pushed the ensure-weather-sdk-codegen-current-ci branch 7 times, most recently from 80d41d8 to d626ac0 Compare May 10, 2024 16:45
@syall syall changed the title feat: add check to see if generated code is current feat: update CI workflow with ordering to reduce costs May 10, 2024
@syall syall force-pushed the ensure-weather-sdk-codegen-current-ci branch from d626ac0 to a946adb Compare May 10, 2024 17:03
Copy link
Contributor

@jbelkins jbelkins left a comment

Choose a reason for hiding this comment

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

Not sure I see the value of this.

Three additional jobs that run in series with our existing CI jobs and duplicate much of the setup we already perform doesn't solve any problem we have. This will just make CI take longer than it already does, which IMO is too long.

If we want to check the diff is clean for generated code, we can do that as an added step in our existing CI steps and it will add very little complexity and very little build time.

@syall syall force-pushed the ensure-weather-sdk-codegen-current-ci branch from a946adb to da4a1e1 Compare May 10, 2024 19:05
@syall syall changed the title feat: update CI workflow with ordering to reduce costs chore: add codegen CI generated code check May 10, 2024
@syall
Copy link
Contributor Author

syall commented May 10, 2024

Not sure I see the value of this.

Three additional jobs that run in series with our existing CI jobs and duplicate much of the setup we already perform doesn't solve any problem we have. This will just make CI take longer than it already does, which IMO is too long.

If we want to check the diff is clean for generated code, we can do that as an added step in our existing CI steps and it will add very little complexity and very little build time.

That make sense, I changed it back to only checking git diff.

@syall syall merged commit fa670c3 into main May 12, 2024
17 checks passed
@syall syall deleted the ensure-weather-sdk-codegen-current-ci branch May 12, 2024 22:27
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