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

Support context path from index file when pushing #14

Merged
merged 2 commits into from
Aug 9, 2018
Merged

Support context path from index file when pushing #14

merged 2 commits into from
Aug 9, 2018

Conversation

shizhMSFT
Copy link
Contributor

@shizhMSFT shizhMSFT commented Aug 8, 2018

When pushing charts to the remote server, the plugin automatically sets the context path from the cached repo index file. If pushing to a URL not a repo or the cache does not exist, the plugin tries to get the index file directly from the server.

Here is an example server generated index file with context path located at http://example.com/some/path/index.yaml with repo url http://example.com/some/path.

apiVersion: v1
serverInfo:
  contextPath: /some/path
entries: {}
generated: "2018-08-08T08:00:00Z"

With this index file, the plugin will send requests to http://example.com/some/path/api/charts for pushing charts instead of http://example.com/api/some/path/charts.

@jdolitsky
Copy link
Contributor

@shizhMSFT this is great! Thanks

Do you mind if we nest this under serverInfo namespace? I imagine there may be more data we will want to know about the server in the future:

apiVersion: v1
serverInfo:
  contextPath: /some/path
entries: {}
generated: "2018-08-08T08:00:00Z"

if err != nil {
return nil, err
}
if resp.StatusCode != 200 {

Choose a reason for hiding this comment

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

This can be before reading body

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's follow the original design. The function call to getChartmuseumError(b, resp.StatusCode) requires reading the response body.

@@ -0,0 +1,48 @@
package helm

Choose a reason for hiding this comment

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

this is the dep package so you should not change it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is pkg not vendor. Therefore, it is not a dep package.

Choose a reason for hiding this comment

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

ah...ok :)

@shizhMSFT
Copy link
Contributor Author

@jdolitsky Nested contextPath under serverInfo namespace.

@yuwaMSFT2
Copy link

LGTM. @jdolitsky can you help to review? Thanks!

@jdolitsky
Copy link
Contributor

Works great! merging

@jdolitsky jdolitsky merged commit 85f5026 into chartmuseum:master Aug 9, 2018
@jdolitsky
Copy link
Contributor

New version of plugin has been released with this feature (v0.7.0)
@shizhMSFT @yuwaMSFT2 @sajayantony

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants