-
Notifications
You must be signed in to change notification settings - Fork 884
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
Feat: implement addon push command #4261
Conversation
Signed-off-by: Charlie Chiang <charlie_c_0129@outlook.com>
Signed-off-by: Charlie Chiang <charlie_c_0129@outlook.com>
Signed-off-by: Charlie Chiang <charlie_c_0129@outlook.com>
Signed-off-by: Charlie Chiang <charlie_c_0129@outlook.com>
Signed-off-by: Charlie Chiang <charlie_c_0129@outlook.com>
Codecov Report
@@ Coverage Diff @@
## master #4261 +/- ##
==========================================
+ Coverage 59.87% 60.55% +0.68%
==========================================
Files 338 339 +1
Lines 32909 33078 +169
==========================================
+ Hits 19703 20032 +329
+ Misses 10594 10419 -175
- Partials 2612 2627 +15
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Signed-off-by: Charlie Chiang <charlie_c_0129@outlook.com>
Signed-off-by: Charlie Chiang <charlie_c_0129@outlook.com>
Signed-off-by: Charlie Chiang <charlie_c_0129@outlook.com>
-----BEGIN PRIVATE KEY----- | ||
MIIJQwIBADANBgkqhkiG9w0BAQEFAASCCS0wggkpAgEAAoICAQDGAvNL5M/Jiv3L | ||
876+cd1Gv/9pAFdqXbCcgOLoxa+ikb6ndR5PTeleVm2xO1Z2FkOdT+RseNIr9Jcx |
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.
Can these crt associated files deleted? Not sure is it good put these files in our code repo.
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.
These files are provided to test support for custom SSL certificates. They are just used for tests. If we remove these files, then TLS-related tests need to be removed as well.
Signed-off-by: Charlie Chiang <charlie_c_0129@outlook.com>
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
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.
great work
p.Out = cmd.OutOrStdout() | ||
p.ChartName = args[0] | ||
p.RepoName = args[1] | ||
p.SetFieldsFromEnv() |
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 there another way other than setting the environment and use flags? What are the ways for helm?
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 think helm has two ways, 1. environment vars (config files, if that counts), 2. command line arguments. We implemented these two as well. The config file way is not implemented.
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.
can we re-use their config if users already have one? read the same defualt path is even better
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 think that config file is appropriate here (so I didn't count it). That config file is from Codefresh CLI
(helm will read its API key), and the repo need to be inside helm
. It stores username and password (tokens, to be precise), so the user don't have to input their credentials every time they push anything.
We are taking advantage of addon registries
, so the same feature is available. If the user add a basic-auth authenticated registry to addon registries
, they don't need to input their credentials when they vela addon push
. Because I will read the credentials from addon registries
.
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.
great, that's I want, but I didn't see the logic in your code.
If the user add a basic-auth authenticated registry to addon registries, they don't need to input their credentials when they vela addon push. Because I will read the credentials from addon registries.
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's here
Line 210 in 229ea19
Username: reg.Helm.Username, |
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.
BUt you're not reading the credentials for certificates
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.
custom certificates are not supported by addon registries
anyway... so I can't really read it
Currently we are only handling basic-auth
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.
anyway, you pointed the right direction we should provide
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.
great work!
Signed-off-by: Charlie Chiang <charlie_c_0129@outlook.com>
Signed-off-by: Charlie Chiang <charlie_c_0129@outlook.com>
Signed-off-by: Charlie Chiang <charlie_c_0129@outlook.com>
Signed-off-by: Charlie Chiang <charlie_c_0129@outlook.com>
Signed-off-by: Charlie Chiang <charlie_c_0129@outlook.com>
Description of your changes
Please refer to this documentation PR to see what it does in detail:
-> kubevela/kubevela.github.io#792
Generally speaking, an
addon push
command is implemented.This command does two things:
.tgz
package, then it skips this step)Examples:
Using Addon directory (
sample-addon
) + repository name (localcm
):Using Addon package (
sample-addon-1.0.0.tgz
) + repository URL (http://localhost:8080
):To make old users that use
helm cm-push
command familiar withaddon push
, while support for KubeVela addons is added, mosthelm cm-push
options are supported as well (check outHelp Text
below).Help Text
This pr includes some features mentioned by @wonderflow in #4116, and is also suggested by @wangyikewxgm
I have:
Docs: add instructions about chartmuseum kubevela.github.io#792
make reviewable
to ensure this PR is ready for review.backport release-x.y
labels to auto-backport this PR if necessary.How has this code been tested
Special notes for your reviewer