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.

Sign up for GitHub

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

Jump to bottom

✨ Initial support for conditions for AzureMachinePool #978

Merged
merged 2 commits into from
Oct 29, 2020
Merged

✨ Initial support for conditions for AzureMachinePool #978

merged 2 commits into from
Oct 29, 2020

Conversation

nprokopic
Copy link
Contributor

@nprokopic nprokopic commented Oct 5, 2020 •
edited by CecileRobertMichon
Loading

What type of PR is this?
/kind feature

What this PR does / why we need it:

Add Status.Conditions to AzureMachinePool.

Status.Conditions were added to AzureCluster and AzureMachine in #714. With this change the resolution of issue #671, which also mentions Status.Conditions for AzureMachinePool, is more complete.

Which issue(s) this PR fixes:
Fixes partially #671

TODOs:

  • squashed commits
  • includes documentation
  • adds unit tests

Release note:

Add `Status.Conditions` field to `AzureMachinePool` showing details about the current state of the object.

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 5, 2020
@k8s-ci-robot
Copy link
Contributor

Hi @nprokopic. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 5, 2020
@nprokopic
Copy link
Contributor Author

/assign @justinsb

@nprokopic nprokopic mentioned this pull request Oct 6, 2020
Merged
@devigned
Copy link
Contributor

devigned commented Oct 6, 2020

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 6, 2020
devigned
devigned suggested changes Oct 6, 2020
View reviewed changes
Copy link
Contributor

@devigned devigned left a comment

Choose a reason for hiding this comment

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

Looks like a bit of copypasta still left over in the comments.

exp/api/v1alpha3/azuremachinepool_types.go Outdated Show resolved Hide resolved
exp/api/v1alpha3/azuremachinepool_types.go Outdated Show resolved Hide resolved
config/crd/bases/exp.infrastructure.cluster.x-k8s.io_azuremachinepools.yaml Outdated Show resolved Hide resolved
exp/api/v1alpha3/azuremachinepool_types.go Outdated Show resolved Hide resolved
@nprokopic
Copy link
Contributor Author

Looks like a bit of copypasta still left over in the comments.

Well that's embarrassing 🤦 Thank you for catching this, fixed it.

@k8s-ci-robot
Copy link
Contributor

@nprokopic: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-cluster-api-provider-azure-apidiff 81728bb link /test pull-cluster-api-provider-azure-apidiff

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@nprokopic
Copy link
Contributor Author

This test failure:

sigs.k8s.io/cluster-api-provider-azure/exp/api/v1alpha3
  Incompatible changes:
  - AzureMachinePoolStatus: old is comparable, new is not
  Compatible changes:
  - (*AzureMachinePool).GetConditions: added
  - (*AzureMachinePool).SetConditions: added
  - AzureMachinePoolStatus.Conditions: added

Is this maybe due to Conditions (added to AzureMachinePoolStatus) being a slice?

Any advice here?

@CecileRobertMichon
Copy link
Contributor

@nprokopic the api diff job helps reviewers catch breaking changes to the API but it's not perfect. In this case, you're change affects the Status field of an experimental type so it's okay to ignore. The job is marked as optional so it won't be blocking to merge the PR.

@nprokopic
Copy link
Contributor Author

@nprokopic the api diff job helps reviewers catch breaking changes to the API but it's not perfect. In this case, you're change affects the Status field of an experimental type so it's okay to ignore.

Thanks for the explanation!

The job is marked as optional so it won't be blocking to merge the PR.

Great :)

CecileRobertMichon
CecileRobertMichon reviewed Oct 6, 2020
View reviewed changes
exp/api/v1alpha3/azuremachinepool_types.go
}

// SetConditions will set the given conditions on an AzureMachinePool object
func (amp *AzureMachinePool) SetConditions(conditions clusterv1.Conditions) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this change also include defining basic conditions for AzureMachinePools and setting them in the controller like https://github.com/kubernetes-sigs/cluster-api-provider-azure/pull/714/files#diff-825ec7ff34f98855f2347c7f7769438dR284 did?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds reasonable. I'll look into it.

Copy link
Contributor

Choose a reason for hiding this comment

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

@nprokopic are you still planning to do this? If not, we can merge this as-is and mark the issue as incomplete

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I do, just couldn't find the time in the previous weeks.

Can we merge this and have a separate pull request for setting conditions?

Lately I was working on conditions implementation for Giant Swarm's azure-operator (where we use CAPI and CAPZ types), here's what we added for current alpha release (so not fully completed yet) for Cluster, AzureCluster and MachinePool giantswarm/azure-operator#1112, and here is current WIP for AzureMachinePool giantswarm/azure-operator#1123.
Once we wrap these up, I would take a look at what's currently done in CAPZ, and then open an issue with suggestions for conditions that would make sense for CAPZ.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay let's do that.

CecileRobertMichon
CecileRobertMichon approved these changes Oct 29, 2020
View reviewed changes
Copy link
Contributor

@CecileRobertMichon CecileRobertMichon left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 29, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CecileRobertMichon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 29, 2020
@k8s-ci-robot k8s-ci-robot merged commit 1d9ad20 into kubernetes-sigs:master Oct 29, 2020
@k8s-ci-robot k8s-ci-robot added this to the v0.4.10 milestone Oct 29, 2020
@stone-z stone-z mentioned this pull request Dec 4, 2020
Closed
@nprokopic nprokopic mentioned this pull request Aug 6, 2021
Closed
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CecileRobertMichon CecileRobertMichon CecileRobertMichon approved these changes

@devigned devigned devigned requested changes

@cpanato cpanato Awaiting requested review from cpanato

@nader-ziada nader-ziada Awaiting requested review from nader-ziada

Assignees

@CecileRobertMichon CecileRobertMichon

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/provider/azure Issues or PRs related to azure provider cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v0.4.10
Development

Successfully merging this pull request may close these issues.
5 participants
@nprokopic @k8s-ci-robot @devigned @CecileRobertMichon @justinsb