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.

Sign up for GitHub

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

Jump to bottom

Leverage az provider list #3707

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Leverage az provider list #3707

wants to merge 11 commits into from

Conversation

danielrbradley
Copy link
Member

@danielrbradley danielrbradley commented Nov 15, 2024 •
edited
Loading

Fixes #2661

  1. Add inactive default versions report - which versions referenced from our default version are not currently marked as "live" by az provider list?
  2. When picking a new tracking version, select from the live versions only, if any are live. Fall back to all versions if az provider list doesn't include any.
  3. When checking for additional resources not in the default version, don't include non-live versions.
  4. Move most unit testing into gen_calculateVersionMetadata_test from defaultVersion_test as it's as close to e2e as possible and requires less mocking.

@danielrbradley danielrbradley self-assigned this Nov 15, 2024
@github-actions GitHub Actions
Copy link

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

@codecov Codecov
Copy link

codecov bot commented Nov 15, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 85.52632% with 11 lines in your changes missing coverage. Please review.

Project coverage is 56.80%. Comparing base (e9325d7) to head (ec126bd).

Files with missing lines Patch % Lines
provider/pkg/providerlist/providerlist.go 83.33% 3 Missing ⚠️
provider/pkg/versioning/defaultVersion.go 93.87% 3 Missing ⚠️
provider/pkg/versioning/build_schema.go 0.00% 2 Missing ⚠️
provider/pkg/versioning/gen.go 66.66% 2 Missing ⚠️
provider/cmd/pulumi-gen-azure-native/main.go 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #3707      +/-   ##
==========================================
+ Coverage   56.41%   56.80%   +0.39%     
==========================================
  Files          74       74              
  Lines       11813    11837      +24     
==========================================
+ Hits         6664     6724      +60     
+ Misses       4652     4617      -35     
+ Partials      497      496       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@danielrbradley
Prevent panic
63ab13f 
We might not have been able to build a partial schema result, so can't write any interim files for debugging.
@danielrbradley
Move and refine 'az provider list' result
02cef39 
- Use built-in `--query` option instead of needing jq installed.
- Move into `versions` directory where all other version data is.
- Add `defaultVersion` and `locations` fields for each resourceType.
@danielrbradley
Simplify sorted versions
dab487d
@danielrbradley
Simplify isMoreThanOneYearOld
ae55f6c 
Use existing openapi.ApiVersionToDate function
@danielrbradley
Pass indexedProviderList into BuildSpec
6517c6f
@danielrbradley
Load 'az provider list' for use in versioning
cff71af 
- Add new report highlighting which versions in the default lock are not listed as live.
- Remove old active report which was the same data as the 'az provider list' but reformatted.
@danielrbradley
Only add additional resources if they're live
dfb03a0
@danielrbradley
Prefer live versions for tracking
e354fc1 
- Fall back to any version if none are live.
- Add documentation comments.
- Create providerSpecBuilder class to give access to global, immutable data across functions to avoid passing providerName and providerList everywhere.
- Simplify filtering candidate versions.

Test adding resources only if they're live
@danielrbradley
Move and extend unit tests
29d5dac 
- Test prefering live versions for tracking
- Combine default version tests
- Move lots of default version tests into the new calculateVersionMetadata tests which are a broader test and require less mocking.
@danielrbradley
Generate schema
7f3adff
@danielrbradley
Generate SDKs
ec126bd
@danielrbradley danielrbradley marked this pull request as ready for review November 26, 2024 16:24
@danielrbradley danielrbradley requested review from thomas11 and a team November 26, 2024 16:24
thomas11
thomas11 reviewed Nov 27, 2024
View reviewed changes
Copy link
Contributor

@thomas11 thomas11 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Pretty tricky to review but overall LGTM. Thanks for the nice test coverage!

Fall back to all versions if as provider list doesn't include any.

Is this in case az provider list is wrong? If it was 100% correct, we could disregard that resource altogether, no?

My main concern is that I don't think we should merge the schema and SDKs with these changes in v2. Even if the Azure API versions are not actually live, it's still a breaking code change. And there's a small chance provider list is wrong.

@danielrbradley
Copy link
Member Author

My main concern is that I don't think we should merge the schema and SDKs with these changes in v2. Even if the Azure API versions are not actually live, it's still a breaking code change. And there's a small chance provider list is wrong.

Agreed. I was also concerned about removing all these. Perhaps we should feature switch the additions removals off until V3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thomas11 thomas11 thomas11 left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@danielrbradley danielrbradley

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use the Azure RP API when determining default API versions
2 participants
@danielrbradley @thomas11