-
Notifications
You must be signed in to change notification settings - Fork 75
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
azure: add virtualmachine extension resource #515
Conversation
/test-examples="examples/compute/windowsvirtualmachine.yaml,examples/compute/linuxvirtualmachine.yaml" |
/test-examples="examples/compute/linuxvirtualmachine.yaml" |
1 similar comment
/test-examples="examples/compute/linuxvirtualmachine.yaml" |
/test-examples="examples/compute/windowsvirtualmachine.yaml,examples/compute/linuxvirtualmachine.yaml" |
hello @turkenf VirtualMachineExtension is a resource that can be used by both check my last 2 commits, i was able to try Resolve for both manually on z_generated_ but after generate it will revert the file as expected Do you know if theres any similar situation like this ? |
Hi @prfj, Thank you for your contribution, I took a look at this PR. First of all, since the resource we added here is
In these cases, you can define a reference manually, you can check here. |
Hey @turkenf , thanks for review, I've added a separated example for virtualmachineextension
I checked that, but in this VirtualMachineExtension case we can reference 2 different type of resources(virtual_machine_id)...it can be a LinuxVirtualMachine or WindowsVirtualMachine...
I think this wont work... I've seen some other issues like this crossplane/crossplane-runtime#350 Seems that when references ref to multiple kinds the option is remove and make the user select the resource without Ref or Selector, example https://github.com/upbound/provider-aws/pull/667/files |
/test-examples="examples/compute/virtualmachineextension.yaml" |
@prfj, you are right, in cases where more than one resource is referenced, we generally do not prefer to define a reference and we pass it manually. But if there is a very common use case, we can define a reference to it. For example, if the common usage for |
/test-examples="examples/compute/windowsvirtualmachine.yaml,examples/compute/linuxvirtualmachine.yaml,examples/compute/virtualmachineextension.yaml" |
1 similar comment
/test-examples="examples/compute/windowsvirtualmachine.yaml,examples/compute/linuxvirtualmachine.yaml,examples/compute/virtualmachineextension.yaml" |
@turkenf when possible please review, i tested on my local and everything worked as expected for examples/compute/windowsvirtualmachine.yaml, examples/compute/linuxvirtualmachine.yaml and examples/compute/virtualmachineextension.yaml i would like to run the uptest here on the MR CI also |
Thank you for your effort in this PR @prfj The test command can only be triggered by Upbound organization members. |
/test-examples="examples/compute/windowsvirtualmachine.yaml" |
/test-examples="examples/compute/linuxvirtualmachine.yaml" |
/test-examples="examples/compute/virtualmachineextension.yaml" |
@turkenf can you understand what happened in uptest ? I tested all resources locally, I think this failed because some resources are repeated on those 3 examples, same nic, rg, subnet, vnet name...can you launch separated tests for each on of the files ? |
/test-examples="examples/compute/windowsvirtualmachine.yaml" |
/test-examples="examples/compute/virtualmachineextension.yaml" |
I could not observe any significant error in the logs. Let me trigger a new test for the |
/test-examples="examples/compute/virtualmachineextension.yaml" |
I've rebased to fix conflicts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@prfj Thank you so much for your contribution, I left two small comments for you to consider.
After making your last changes, I will trigger the uptests again, and if the tests are green will merge.
/test-examples="examples/compute/windowsvirtualmachine.yaml" |
@ulucinar please re run the test examples and review the PR when possible thanks |
/test-examples="examples/compute/windowsvirtualmachine.yaml" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @prfj, LGTM.
Description of your changes
Fixes #514
I have:
make reviewable test
to ensure this PR is ready for review.How has this code been tested
Tested with uptest: https://github.com/upbound/provider-azure/actions/runs/6262156775