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

version: bump ec2/vpccidrblock package to manualv1alpha1 #751

Merged
merged 1 commit into from
Jul 13, 2021

Conversation

haarchri
Copy link
Member

@haarchri haarchri commented Jun 29, 2021

Signed-off-by: Christopher Haar chhaar30@googlemail.com

bump package version for ec2/vpccidrblock to manualv1alpha1 to use v1alpha1 for code-generator stuff
in #689 we see the problem between custom and code-generator stuff in one - for example: of zz_types.go
think it is better not mix-up the both approaches

what did you guys think ?

I have:

  • Read and followed Crossplane's contribution process.
  • Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

➜  provider-aws git:(ec2-vpccidrblock) ✗ kubectl apply -f examples/ec2/vpccidrblock.yaml
vpccidrblock.ec2.aws.crossplane.io/sample-vpc-cidr-block created
➜  provider-aws git:(ec2-vpccidrblock) ✗ kubectl delete -f examples/ec2/vpccidrblock.yaml
vpccidrblock.ec2.aws.crossplane.io "sample-vpc-cidr-block" deleted

@AaronME
Copy link
Collaborator

AaronME commented Jun 30, 2021

I see what you are saying. And I agree that it would be nice to have a distinction between the generated and non-generated types. Could using alpha2 for non-generated create confusion? Someone could infer that alpha2 indicates a more recent, and preferred, approach. When really we prefer people to follow practices in alpha1.

@haarchri
Copy link
Member Author

haarchri commented Jul 5, 2021

so what is your prefered way ? at the moment we need to modify in ec2 zz_ files if we want integrate more services with code-generator

Copy link
Member

@hasheddan hasheddan left a comment

Choose a reason for hiding this comment

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

@haarchri we don't need to actually change the API version, we can just change the package name so that the generated code lives in a different package. Users of this API should have the same experience as they do currently 👍🏻

@haarchri
Copy link
Member Author

haarchri commented Jul 7, 2021

@hasheddan can you provide a example ? At the moment the code-generator will add in ec2 v1alpha1

Copy link
Member

@hasheddan hasheddan left a comment

Choose a reason for hiding this comment

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

@haarchri left some guiding comments below, let me know if you want to sync up about them if they aren't illustrating my suggestions well 👍🏻

apis/ec2/v1alpha2/referencers.go Outdated Show resolved Hide resolved
apis/ec2/v1alpha2/register.go Outdated Show resolved Hide resolved
@haarchri haarchri changed the title version: bump ec2/vpccidrblock to v1alpha2 version: bump ec2/vpccidrblock package to manualv1alpha1 Jul 13, 2021
@haarchri
Copy link
Member Author

@hasheddan i changed the packag to manualv1alpha1 and the generated crds using v1alpha1 - think this was this what you mentioned ;)

tested via apply and delete

for delete i added a fix that the ressource is removed when vpccidrblock is in status VpcCidrBlockStateCodeDisassociated
you can have a look

Signed-off-by: Christopher Haar <chhaar30@googlemail.com>
Copy link
Member

@hasheddan hasheddan left a comment

Choose a reason for hiding this comment

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

Thank you @haarchri !

@hasheddan hasheddan merged commit d4634fb into crossplane-contrib:master Jul 13, 2021
@haarchri haarchri mentioned this pull request Oct 21, 2021
6 tasks
tektondeploy pushed a commit to gtn3010/provider-aws that referenced this pull request Mar 12, 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.

None yet

3 participants