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

Bug 1784582: Added Helm CLI download links #362

Merged
merged 1 commit into from Jan 3, 2020

Conversation

pedjak
Copy link
Contributor

@pedjak pedjak commented Dec 10, 2019

This PR adds to OpenShift Console the additional Helm CLI download link(s).

@openshift-ci-robot openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Dec 10, 2019
@pedjak
Copy link
Contributor Author

pedjak commented Dec 10, 2019

Screenshot-20191210192800-976x545

After merging this request, the CLI download page will look as above.

@pedjak
Copy link
Contributor Author

pedjak commented Dec 10, 2019

/assign @benjaminapetersen

@sbose78
Copy link

sbose78 commented Dec 10, 2019

CC @siamaksade @bparees @soltysh

@pedjak
Copy link
Contributor Author

pedjak commented Dec 11, 2019

/retest

@pedjak
Copy link
Contributor Author

pedjak commented Dec 11, 2019

It looks that tests are failing due to infra issues:

Error 400: The number of members in the policy (1,501) is larger than the maximum allowed size 1,500., badRequest"

How to make them pass?

@pedjak
Copy link
Contributor Author

pedjak commented Dec 11, 2019

/retest

Copy link
Member

@jhadvig jhadvig left a comment

Choose a reason for hiding this comment

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

The PR itself looks good 👍
Please address the working suggested in https://github.com/openshift/console-operator/pull/362/files#r356387619 so we can merge.

@benjaminapetersen thoughts ?

description: |
Helm 3 is a package manager for Kubernetes applications which enables defining,
installing and upgrading applications packaged as Helm Charts.
Helm 3 is in <a href=https://access.redhat.com/support/offerings/techpreview>Technology Preview</a> feature on OpenShift 4.
Copy link

@sbose78 sbose78 Dec 12, 2019

Choose a reason for hiding this comment

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

Predrag, have you verified that it shows up as a clickable URL?

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, see the screenshot attached above,

Copy link
Contributor

Choose a reason for hiding this comment

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

either:
Helm 3 is in Technology Preview on OpenShift
or
Helm 3 is a Technology Preview feature on Openshift

but Helm 3 is in Technology Preview feature on OpenShift is not a grammatically correct sentence.

Copy link
Contributor

Choose a reason for hiding this comment

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

There is also an e2e suite in console that verifies the CLIDownload links. You could add a few lines here as well:

https://github.com/openshift/console/blob/e3080430f4ac297619b57062410c461333e62c9f/frontend/integration-tests/tests/crd-extensions.scenario.ts#L15

@bparees
Copy link
Contributor

bparees commented Dec 13, 2019

@sbose78 @pedjak https://mirror.openshift.com/pub/openshift-v4/helm/latest is not a valid link. When do you plan to have the content there? I do not want us to ship ocp4 with a broken link in the console.

also i would expect the url to be something under https://mirror.openshift.com/pub/openshift-v4/clients/

@bparees
Copy link
Contributor

bparees commented Dec 13, 2019

/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 13, 2019
@pedjak
Copy link
Contributor Author

pedjak commented Dec 13, 2019

@bparees thanks, correct the grammar issue and changed the download link to https://mirror.openshift.com/pub/openshift-v4/clients/helm/latest My understanding is that we want to deliver helm with 4.3 release, right @sbose78 ?

@bparees
Copy link
Contributor

bparees commented Dec 13, 2019

/hold cancel
@benjaminapetersen @spadgett i think this can merge if you're ok with it, assuming we can get confirmation the helm binary will be there by the time 4.3 GAs.

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 13, 2019
@sbose78
Copy link

sbose78 commented Dec 13, 2019

Yes, that's correct, Predrag & Ben. The binary would be there when 4.3 releases.

@benjaminapetersen
Copy link
Contributor

You could also update the tests
https://github.com/openshift/console-operator/blob/master/test/e2e/downloads_test.go#L24
To prove that the link functions in 4.3/4.4.

@benjaminapetersen
Copy link
Contributor

We should also squash the commits before merge.

Copy link
Contributor

@benjaminapetersen benjaminapetersen left a comment

Choose a reason for hiding this comment

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

Just a few comments. Definitely a squash, and ideally add to the tests.

@pedjak
Copy link
Contributor Author

pedjak commented Dec 16, 2019

@benjaminapetersen I would add the check to the tests, but we still do not have the binaries ready for the download, or you mean that we should only check if the link gets rendered? BTW, Checking ODO links is also not a part of the test atm.

I have squashed the commits.

manifests/07-downloads-helm.yaml Outdated Show resolved Hide resolved
manifests/07-downloads-helm.yaml Outdated Show resolved Hide resolved
manifests/07-downloads-helm.yaml Outdated Show resolved Hide resolved
@pedjak
Copy link
Contributor Author

pedjak commented Dec 17, 2019

@spadgett - I have included the proposed fixes, thanks for your feedback. I cannot add the test at the moment because we do not have binaries ready for download. Speaking about the links, should we add actually individual DL links for each of the platforms (linux, osx, windows) or leave it as it is. I have followed ODO approach, @siamaksade what do you think?

Speaking about testing, the existing TestDownloadsEndpoint() checks only if oc can be downloaded. Do we have somewhere browser tests that check if the page is properly rendered? Do we use https://github.com/tebeka/selenium or similar?

@bparees
Copy link
Contributor

bparees commented Dec 17, 2019

fyi we are passed code freeze, this did not merge. Barring a request for an extremely urgent exception, this will not ship in 4.3.0GA, we can consider including in in the first z-stream.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

17 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@bparees
Copy link
Contributor

bparees commented Dec 20, 2019

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 20, 2019
@pedjak
Copy link
Contributor Author

pedjak commented Dec 23, 2019

@bparees the test were passing the last time here but the only big difference to the actual PR is that Tech Preview link was declared using <a href.../> syntax.

@benjaminapetersen
Copy link
Contributor

The console e2e test suite is struggling atm, we are definitely looking into it.

@spadgett
Copy link
Member

spadgett commented Jan 2, 2020

We're working on a fix for the console e2e tests in openshift/console#3847

@spadgett
Copy link
Member

spadgett commented Jan 3, 2020

/hold cancel
/retest
CI is fixed

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 3, 2020
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 3998db4 into openshift:master Jan 3, 2020
@openshift-ci-robot
Copy link
Contributor

@pedjak: All pull requests linked via external trackers have merged. Bugzilla bug 1784582 has been moved to the MODIFIED state.

In response to this:

Bug 1784582: Added Helm CLI download links

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.

@openshift-cherrypick-robot

@pedjak: new pull request created: #363

In response to this:

/cherry-pick release-4.3

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.

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. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet