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

Add new pipeline stage that merges groups #1579

Merged
merged 2 commits into from
Jun 24, 2021

Conversation

matthchr
Copy link
Member

Fixes #1578

What this PR does / why we need it:

  • In the case that group A references group B, we need to pull group B's types into group A.
  • Also fixes a bug in the Crossplane golden files tests which combined with this new pipeline stage could cause all types to get pruned.

@codecov-commenter
Copy link

codecov-commenter commented Jun 19, 2021

Codecov Report

Merging #1579 (9559824) into master (f60f783) will increase coverage by 0.01%.
The diff coverage is 71.42%.

❗ Current head 9559824 differs from pull request most recent head 9fde653. Consider uploading reports for the commit 9fde653 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1579      +/-   ##
==========================================
+ Coverage   63.41%   63.42%   +0.01%     
==========================================
  Files         181      182       +1     
  Lines       11797    11818      +21     
==========================================
+ Hits         7481     7496      +15     
- Misses       3648     3651       +3     
- Partials      668      671       +3     
Impacted Files Coverage Δ
.../pkg/codegen/pipeline_collapse_cross_group_refs.go 70.00% <70.00%> (ø)
hack/generator/pkg/codegen/code_generator.go 88.18% <100.00%> (+0.09%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f60f783...9fde653. Read the comment docs.

Copy link
Member

@theunrepentantgeek theunrepentantgeek left a comment

Choose a reason for hiding this comment

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

One question that we need to answer before merging, otherwise good.

@matthchr matthchr force-pushed the feature/collapse-groups branch from 9fde653 to 9b7a8b9 Compare June 21, 2021 17:16
@matthchr matthchr added this to the codegen-alpha-0 milestone Jun 21, 2021
@matthchr matthchr force-pushed the feature/collapse-groups branch from 9b7a8b9 to 7939833 Compare June 23, 2021 18:25
Copy link
Member

@babbageclunk babbageclunk left a comment

Choose a reason for hiding this comment

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

Pretty neat use of TypeWalker!

if !resourceName.PackageReference.Equals(updated.Name().PackageReference) {
// Note: If we ever find this generating colliding names, we might need to introduce a unique suffix.
// For now though it doesn't seem to, so preserving the shorter names as they're clearer.
updated = updated.WithName(astmodel.MakeTypeName(resourceName.PackageReference, updated.Name().Name()))
Copy link
Member

Choose a reason for hiding this comment

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

Presumably in the case of a collision we'll panic rather than just overwriting?

Copy link
Member Author

Choose a reason for hiding this comment

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

A collision would be err := result.AddAllowDuplicates(newDef) above returning an error - meaning we had two types that were named the same but are structurally different. Right now that'd just block the whole pipeline and we'd have to come and add handling to deal with it.

  - In the case that group A references group B, we need to
    pull group B's types into group A.
  - Fixes Azure#1578
@matthchr matthchr force-pushed the feature/collapse-groups branch from 7939833 to 1e00474 Compare June 23, 2021 22:17
@matthchr matthchr merged commit d420499 into Azure:master Jun 24, 2021
@matthchr matthchr deleted the feature/collapse-groups branch June 24, 2021 17:20
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.

StorageTypeFactory assumes that types do not reference one another across group boundaries
4 participants