-
Notifications
You must be signed in to change notification settings - Fork 65
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
PublicIPAddress support #264
PublicIPAddress support #264
Conversation
o := managed.ExternalObservation{ | ||
ResourceExists: true, | ||
ConnectionDetails: managed.ConnectionDetails{}, | ||
} | ||
|
||
return o, nil |
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.
o := managed.ExternalObservation{ | |
ResourceExists: true, | |
ConnectionDetails: managed.ConnectionDetails{}, | |
} | |
return o, nil | |
return managed.ExternalObservation{ | |
ResourceExists: true, | |
ResourceUpToDate: true, | |
}, nil |
We return ResourceUpToDate: true
because I see that nothing can be updated in publicipaddress.external.Update
. Is this really the case, can no fields be updated? What about tags?
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.
I've add todo about update support. It's not very often case for now
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.
Thank you @vaspahomov for your work on this PR! Could you please rebase this PR and make sure all your commits are signed following the guide here. We also require them to be signed-off using something similar to git commit -s
. git log --show-signature
should display commit signatures.
1baed27
to
aaaf3e9
Compare
Hi @ulucinar thank you! I've commit fixes |
Hi @vaspahomov,
It would be good if you could also remove the duplicate sign-off. |
|
||
// ResourceGroupName - Name of the Public IP address's resource group. | ||
// +immutable | ||
ResourceGroupName string `json:"resourceGroupName,omitempty"` |
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.
Hi @vaspahomov,
Per Crossplane API conventions, could you please move parameter fields here to their own PublicIPAddressParameters
struct and add them under spec.forProvider
here. For example:
https://github.com/crossplane/provider-azure/blob/3a2c932bb9e0557b72541f2ad851666b645de4cf/apis/database/v1beta1/configuration_types.go#L103
aaaf3e9
to
0d7c878
Compare
Hi @vaspahomov, |
0d7c878
to
7bc8d01
Compare
Signed-off-by: vaspahomov <vaspahomov@yandex-team.ru>
7bc8d01
to
de13fd3
Compare
Thank you very much @vaspahomov, merging this PR. |
@vaspahomov, I've just opened a follow-up PR as discussed. |
Hello!
I want to add VM instances support in crossplane/provider-azure.
This is first part of big patch with Public IP addresses support
Description of your changes
Add support of Public IP Address.
Fixes #208
I have:
make reviewable test
to ensure this PR is ready for review.How has this code been tested
Same like in other resources like Subnet and VisrtualNetwork. And also manually tested