-
Notifications
You must be signed in to change notification settings - Fork 670
Bug 1878016: Add documentation to describe configuring multiple helm chart repos #6324
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 1878016: Add documentation to describe configuring multiple helm chart repos #6324
Conversation
|
/test e2e-gcp-console |
08b048a to
614d93c
Compare
jhadvig
left a comment
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.
Adding couple of questions. Not entirely familiar with Helm so need to clear this out.
docs/helm/endpoints_api.md
Outdated
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.
Shouldn't here be a 404 as well ?
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.
might be, but that is not in the focus of PR - we have just extracted the existing doc into a separate file.
docs/helm/endpoints_api.md
Outdated
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.
This doesn't seems right, since 502 RC should be used in case of server error response code indicates that the server, while acting as a gateway or proxy, received an invalid response from the upstream server.
400 and 404 should be used here.
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.
might be, but that is not in the focus of PR - we have just extracted the existing doc into a separate file.
docs/helm/endpoints_api.md
Outdated
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.
Same as above
docs/helm/endpoints_api.md
Outdated
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.
same as above
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.
might be, but that is not in the focus of PR - we have just extracted the existing doc into a separate file. If we need to do something, let's t do that in a separate PR, because that would also require the change of endpoint logic, not just the doc.
docs/helm/endpoints_api.md
Outdated
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.
same
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.
might be, but that is not in the focus of PR - we have just extracted the existing doc into a separate file. If we need to do something, let's t do that in a separate PR, because that would also require the change of endpoint logic, not just the doc.
docs/helm/endpoints_api.md
Outdated
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.
same
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.
might be, but that is not in the focus of PR - we have just extracted the existing doc into a separate file. If we need to do something, let's t do that in a separate PR, because that would also require the change of endpoint logic, not just the doc.
|
/test e2e-gcp-console |
jhadvig
left a comment
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.
Thanks @pedjak for explanation... one last styling nit.
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.
| When requesting repo index through `/api/helm/charts/index.yaml endpoint, `HelmChartRepository` CRs and | |
| When requesting repo index through `/api/helm/charts/index.yaml` endpoint, `HelmChartRepository` CRs and |
614d93c to
41121a3
Compare
jhadvig
left a comment
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.
/lgtm
|
@akashshinde: This pull request references Bugzilla bug 1878016, which is invalid:
Comment DetailsIn response to this:
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. |
|
/bugzilla refresh |
|
@akashshinde: This pull request references Bugzilla bug 1878016, which is invalid:
Comment DetailsIn response to this:
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. |
|
/bugzilla refresh |
|
@akashshinde: This pull request references Bugzilla bug 1878016, which is invalid:
Comment DetailsIn response to this:
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. |
|
/bugzilla refresh |
|
@akashshinde: This pull request references Bugzilla bug 1878016, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
DetailsIn response to this:
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. |
|
@jhadvig can you approve this ? |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: akashshinde, jhadvig, spadgett The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@akashshinde: All pull requests linked via external trackers have merged: Bugzilla bug 1878016 has been moved to the MODIFIED state. DetailsIn response to this:
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. |
This PR adds documentation for the feature introduced in #5933