[go: nahoru, domu]

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

azure: add virtualmachine extension resource #515

Merged
merged 5 commits into from
Sep 22, 2023
Merged

azure: add virtualmachine extension resource #515

merged 5 commits into from
Sep 22, 2023

Conversation

prfj
Copy link
Contributor
@prfj prfj commented Aug 11, 2023

Description of your changes

Fixes #514

I have:

  • [ X ] Run 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

@Upbound-CLA
Copy link
Upbound-CLA commented Aug 11, 2023

CLA assistant check
All committers have signed the CLA.

@turkenf
Copy link
Collaborator
turkenf commented Aug 11, 2023

/test-examples="examples/compute/windowsvirtualmachine.yaml,examples/compute/linuxvirtualmachine.yaml"

@prfj
Copy link
Contributor Author
prfj commented Aug 11, 2023

/test-examples="examples/compute/linuxvirtualmachine.yaml"

1 similar comment
@turkenf
Copy link
Collaborator
turkenf commented Aug 11, 2023

/test-examples="examples/compute/linuxvirtualmachine.yaml"

@prfj
Copy link
Contributor Author
prfj commented Aug 17, 2023

/test-examples="examples/compute/windowsvirtualmachine.yaml,examples/compute/linuxvirtualmachine.yaml"

@prfj
Copy link
Contributor Author
prfj commented Aug 17, 2023

hello @turkenf

VirtualMachineExtension is a resource that can be used by both LinuxVirtualMachine and WindowsVirtualMachine, how we can allow something like this in the ResolveReferences ?

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 ?

@turkenf
Copy link
Collaborator
turkenf commented Aug 17, 2023

Hi @prfj,

Thank you for your contribution, I took a look at this PR. First of all, since the resource we added here is azurerm_virtual_machine_extension, we should add examples/compute/virtualmachineextension.yaml and test it. Do you know about this guide, it might help.

Do you know if theres any similar situation like this ?

In these cases, you can define a reference manually, you can check here.
As an example: https://github.com/upbound/provider-azure/blob/f546723d3439fcd4b67c2130ac34e1690d807df7/config/compute/config.go#L16

@prfj
Copy link
Contributor Author
prfj commented Aug 18, 2023

Hey @turkenf , thanks for review,

I've added a separated example for virtualmachineextension

In these cases, you can define a reference manually, you can check here. As an example:

https://github.com/upbound/provider-azure/blob/f546723d3439fcd4b67c2130ac34e1690d807df7/config/compute/config.go#L16

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...

		r.References["virtual_machine_id"] = config.Reference{
			Type:      "LinuxVirtualMachine",
			Extractor: rconfig.ExtractResourceIDFuncPath,
		}

		r.References["virtual_machine_id"] = config.Reference{
			Type:      "WindowsVirtualMachine",
			Extractor: rconfig.ExtractResourceIDFuncPath,
		}

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

@prfj
Copy link
Contributor Author
prfj commented Aug 18, 2023

/test-examples="examples/compute/virtualmachineextension.yaml"

@turkenf
Copy link
Collaborator
turkenf commented Aug 18, 2023

@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 virtual_machine_id is LinuxVirtualMachine this can stay, and if we want to use WindowsVirtualMachine resource we can pass virtualMachineId: ........ manually.

@prfj
Copy link
Contributor Author
prfj commented Aug 18, 2023

/test-examples="examples/compute/windowsvirtualmachine.yaml,examples/compute/linuxvirtualmachine.yaml,examples/compute/virtualmachineextension.yaml"

1 similar comment
@jeanduplessis
Copy link
Collaborator

/test-examples="examples/compute/windowsvirtualmachine.yaml,examples/compute/linuxvirtualmachine.yaml,examples/compute/virtualmachineextension.yaml"

@prfj
Copy link
Contributor Author
prfj commented Aug 18, 2023

@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

@turkenf
Copy link
Collaborator
turkenf commented Aug 18, 2023

Thank you for your effort in this PR @prfj

The test command can only be triggered by Upbound organization members.

@turkenf
Copy link
Collaborator
turkenf commented Aug 18, 2023

/test-examples="examples/compute/windowsvirtualmachine.yaml"

@turkenf
Copy link
Collaborator
turkenf commented Aug 18, 2023

/test-examples="examples/compute/linuxvirtualmachine.yaml"

@turkenf
Copy link
Collaborator
turkenf commented Aug 18, 2023

/test-examples="examples/compute/virtualmachineextension.yaml"

@prfj
Copy link
Contributor Author
prfj commented Aug 21, 2023

@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 ?

@sergenyalcin
Copy link
Collaborator

/test-examples="examples/compute/windowsvirtualmachine.yaml"

@sergenyalcin
Copy link
Collaborator

/test-examples="examples/compute/virtualmachineextension.yaml"

@sergenyalcin
Copy link
Collaborator

I could not observe any significant error in the logs. Let me trigger a new test for the virtualmachineextension resource. If it again fails, we may try to put a long timeout to handle the problem.

@sergenyalcin
Copy link
Collaborator

/test-examples="examples/compute/virtualmachineextension.yaml"

@prfj
Copy link
Contributor Author
prfj commented Aug 23, 2023

I've rebased to fix conflicts

Copy link
Collaborator
@turkenf turkenf left a 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.

config/provider.go Show resolved Hide resolved
examples/compute/windowsvirtualmachine.yaml Outdated Show resolved Hide resolved
@turkenf
Copy link
Collaborator
turkenf commented Sep 18, 2023

/test-examples="examples/compute/windowsvirtualmachine.yaml"

@prfj
Copy link
Contributor Author
prfj commented Sep 21, 2023

@ulucinar please re run the test examples and review the PR when possible thanks

@turkenf
Copy link
Collaborator
turkenf commented Sep 21, 2023

/test-examples="examples/compute/windowsvirtualmachine.yaml"

Copy link
Collaborator
@turkenf turkenf left a comment

Choose a reason for hiding this comment

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

Thanks @prfj, LGTM.

@turkenf turkenf merged commit 2cfa85f into crossplane-contrib:main Sep 22, 2023
10 checks passed
@prfj prfj deleted the feat-vm-extension branch September 22, 2023 09:06
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.

Request for azurerm_virtual_machine_extension resource
5 participants