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

Cm beta.6, a plugin for Red Hat Advanced Cluster Management #1618

Merged
merged 9 commits into from
Oct 13, 2021

Conversation

itdove
Copy link
Contributor

@itdove itdove commented Oct 11, 2021

No description provided.

Signed-off-by: Dominique Vernier <dvernier@redhat.com>
Signed-off-by: Dominique Vernier <dvernier@redhat.com>
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 11, 2021
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 11, 2021
@itdove itdove changed the title Cm beta.6 Cm beta.6, a plugin for Red Hat Advanced Managed Cluster Oct 11, 2021
plugins/cm.yaml Outdated Show resolved Hide resolved
plugins/cm.yaml Outdated Show resolved Hide resolved
@ahmetb
Copy link
Member

ahmetb commented Oct 12, 2021

I see the project is new and you folks chose the name "cm". It doesn't go against our guidelines but wanna make sure it doesn't hurt discoverability for you.

Signed-off-by: Dominique Vernier <dvernier@redhat.com>
@itdove
Copy link
Contributor Author

itdove commented Oct 13, 2021

@ahmetb Thanks for the advice, we good with that short name as we think people will discover the CLI through the product and not the other way around.
Many thanks

@itdove itdove requested a review from ahmetb October 13, 2021 03:17
Signed-off-by: Dominique Vernier <dvernier@redhat.com>
@itdove itdove changed the title Cm beta.6, a plugin for Red Hat Advanced Managed Cluster Cm beta.6, a plugin for Red Hat Advanced Cluster Management Oct 13, 2021
Signed-off-by: Dominique Vernier <dvernier@redhat.com>
@itdove
Copy link
Contributor Author

itdove commented Oct 13, 2021

@ahmetb The test failed because I had a typo on ppc64le, fixed it.

@itdove
Copy link
Contributor Author

itdove commented Oct 13, 2021

@ahmetb You don't support this platform:

1962 main.go:62] spec.platform[3]'s selector (&LabelSelector{MatchLabels:map[string]string{arch: ppc64le,os: linux,},MatchExpressions:[]LabelSelectorRequirement{},}) doesn't match any supported platforms

however it is in the list

go tool dist list

@ahmetb
Copy link
Member

ahmetb commented Oct 13, 2021

We don’t support krew on all platforms yet.
That’s why it wouldn’t be installed via Krew. Does that make sense?

@itdove
Copy link
Contributor Author

itdove commented Oct 13, 2021

@ahmetb makes sense but the doc mentions

The possible values for os and arch come from the Go runtime. Run go tool dist list to see all possible platforms and architectures.

at https://krew.sigs.k8s.io/docs/developer-guide/plugin-manifest/

is the ppc64le the only one I listed in the manifest which is not supported or there is some others?
Removing the ppc64le...

Signed-off-by: Dominique Vernier <dvernier@redhat.com>
@ahmetb
Copy link
Member

ahmetb commented Oct 13, 2021

You're definitely right!

Krew does support installing to that platform (e.g. you can override the detected platform via KREW_OS and KREW_ARCH). But it seems our manifest validation tool for krew-index repo checks for the platforms Krew itself is distributed for. https://github.com/kubernetes-sigs/krew/blob/b75099469b59e82d2ca3336f13cb32a498063c78/cmd/validate-krew-manifest/main.go#L230-L244

I think we should maybe relax this or revisit the docs. That said, people don't hit this much yet.

It seems that you have another validation failure now. (s390 is also not supported.)

Signed-off-by: Dominique Vernier <dvernier@redhat.com>
@itdove
Copy link
Contributor Author

itdove commented Oct 13, 2021

@ahmetb Thank you, no worries, I removed s390... if really necessary end-user can download directly the release.
thx

@ahmetb
Copy link
Member

ahmetb commented Oct 13, 2021

/lgtm
/approve

Also, please consider setting up Krew release automation which helps you skip manually making updates to your Krew manifests on each new version and send a pull request. It’s a GitHub workflow bot that can run every time you push a new tag. Those PRs are automatically approved.

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahmetb, itdove

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 13, 2021
Signed-off-by: Dominique Vernier <dvernier@redhat.com>
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 13, 2021
@itdove
Copy link
Contributor Author

itdove commented Oct 13, 2021

@ahmetb Retriger because missing the LICENSE

@ahmetb
Copy link
Member

ahmetb commented Oct 13, 2021

Windows platform has an issue (see test logs).

Signed-off-by: Dominique Vernier <dvernier@redhat.com>
@itdove
Copy link
Contributor Author

itdove commented Oct 13, 2021

@ahmetb now we good ;-)

@ahmetb
Copy link
Member

ahmetb commented Oct 13, 2021

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 13, 2021
@k8s-ci-robot k8s-ci-robot merged commit 8ff661b into kubernetes-sigs:master Oct 13, 2021
@itdove itdove deleted the cm-beta.4 branch October 14, 2021 09:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants