-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Alibaba: Update tag and API types to mirror other providers #5381
Alibaba: Update tag and API types to mirror other providers #5381
Conversation
8632a9c
to
2419362
Compare
I am going to assume that this is a WIP PR since it includes numerous disjoint changes and a vendor of a personal repo. /retitle [WIP] Alibaba: Update tag types to mirror other providers |
@staebler Thanks, I should have tidied this up. I'm waiting for the mapi to update and then this one is eligible. |
2419362
to
0b12357
Compare
0b12357
to
a91dda4
Compare
@staebler @patrickdillon I was awaiting changes to land in the cluster-api-provider-alibaba repository (openshift/cluster-api-provider-alibaba#12). These changes landed and in order to get the changes to fit I had to refactor and remove the machine api libraries from the other providers. Please let me know if you have any questions. Thanks! |
Seems like cluster was still installing but test timed out (ovirt)?
|
/test e2e-aws-upgrade |
/retest |
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.
The last commit seems unrelated to tagging.
The other commits could use some elaboration in their commits messages. For example, "update and simplify tag types" does not tell me anything about how or why the tag types are being updated or simplified.
pkg/asset/machines/master.go
Outdated
@@ -544,7 +542,7 @@ func (m *Master) Machines() ([]machineapi.Machine, error) { | |||
libvirtapi.AddToScheme(scheme) | |||
openstackapi.AddToScheme(scheme) | |||
ovirtproviderapi.AddToScheme(scheme) | |||
vsphereapi.AddToScheme(scheme) | |||
machineapi.AddToScheme(scheme) |
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.
See #5350 (review) for why this won't work.
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.
@staebler Since there are multiple PRs attempting to solve this issue, what do you recommend?
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.
@kirankt is working on it. I recommend waiting for his fix. Is this something that is blocking this PR?
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.
@staebler The issue here is that the cluster-api-provider-alibaba requires a newer version of the machine-api-operator. The vsphere provider requires an older version. Those conflict based upon the machine libraries moving to openshift/api. There might be some go.mod pinning to work around this but I wanted to provide the correct fix by cleaning up the installer dependencies and moving the requirement to openshift/api. I will wait to see what @kirankt comes up with unless you have a suggestion.
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 don't have a good solution. The recent vendoring brought back MAO probably because of cap-alibaba deps.
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.
@kirankt The issue here isn't cap-Alibaba. The dependency for mao in cap-Alibaba is recent. The issue is we need to clean up the vsphere libraries. The vsphere and openstack caps depend on the older version. I created merged PRs in both of those repositories to update the mao dependency.
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.
a91dda4
to
d7d5876
Compare
d7d5876
to
0b17090
Compare
Awaiting #5350 and then will clean this PR up. |
0b17090
to
440bd1e
Compare
440bd1e
to
5b62e9e
Compare
// +optional | ||
SystemDiskSize int `json:"systemDiskSize,omitempty"` | ||
SystemDisk machinev1.SystemDiskProperties `json:"systemDisk,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.
Is this change intentional? There is no mention of it in the commit description. Generally, it is discouraged to use a type defined elsewhere in the install config. Doing so makes it much more difficult to ensure the API stability of the install config.
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.
It was intentional to align with the new API types. I have gone back and replaced it with what was previously there from your comments.
go.mod
Outdated
gopkg.in/yaml.v2 v2.4.0 | ||
k8s.io/api v0.22.2 | ||
k8s.io/apiextensions-apiserver v0.21.0-rc.0 | ||
k8s.io/apiextensions-apiserver v0.22.0-rc.0 |
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.
If we are changing this anyway, we may as well change it to v0.22.2 so that it matches the version of k8s.io/api that we are using.
k8s.io/apiextensions-apiserver v0.22.0-rc.0 | |
k8s.io/apiextensions-apiserver v0.22.2 |
dfdb48a
to
556d97e
Compare
556d97e
to
25fc197
Compare
/lgtm |
Updated vendor to resolve conflicts. |
/lgtm cancel |
go.mod
Outdated
github.com/openshift/machine-config-operator v0.0.0 | ||
github.com/ovirt/go-ovirt v0.0.0-20210308100159-ac0bcbc88d7c | ||
github.com/ovirt/terraform-provider-ovirt v0.99.1-0.20211019085223-db1ac552ec57 | ||
github.com/pborman/uuid v1.2.0 | ||
github.com/pkg/errors v0.9.1 | ||
github.com/pkg/sftp v1.10.1 | ||
<<<<<<< HEAD |
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.
Unresolved 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.
I noticed this moved to go 1.17. Updated.
Thanks!
…ating ovirt and openstack. Remove AliyunContainerService/ to openshift/.
…brary. The updates include renaming and replacing the ResourceTagReference as well as modifying the SystemDisk property. The tag update changes the type to an AlibabaResourceReference which is a struct that contains an ID field and a slice of tags. This reduces the need for two separate API fields and simplifies the logic into a single field. The reference can hold the information in which to search for a component inside of the provider or reference it by ID. The SystemDisk info update is an attempt to reduce the surface area of the API and consolidate all information of the SystemDisk into a single struct.
25fc197
to
03b791c
Compare
/test e2e-aws-upgrade |
@kwoodson: The following tests failed, say
Full PR test history. Your PR dashboard. 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. |
/test e2e-aws-upgrade |
@staebler I fixed the rebase issues from the previous go 1.17 update. Please TAL |
This looks good to me. I'd like to see a successful openstack and ovirt e2e job. The e2e-ovirt history looks pretty bad, unrelated to this PR. We should be able to get a successful openstack job (note that the openshift-kuryr job was successful). /test e2e-openstack |
@staebler Looks like e2e-{ovirt,openstack} are now passing. Mind bumping this along? |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: staebler 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 |
/retest-required Please review the full test history for this PR and help us cut down flakes. |
2 similar comments
/retest-required Please review the full test history for this PR and help us cut down flakes. |
/retest-required Please review the full test history for this PR and help us cut down flakes. |
Hi @kwoodson Is there any special use for specifying the version of gophercloud in gomod? I noticed that you updated the version of gophercloud and then specified the version of gophercloud below. |
Work summary:
*.apps
. This is created by the ingress-operator.