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

Add secret list command to faas-cli #578

Merged
merged 4 commits into from Jan 10, 2019

Conversation

Projects
None yet
4 participants
@viveksyngh
Copy link
Member

viveksyngh commented Jan 5, 2019

This commit adds faas-cli secret list command to the faas-cli which
will be used to get secret list from the cluster.

Signed-off-by: Vivek Singh vivekkmr45@yahoo.in

Description

Motivation and Context

  • I have raised an issue to propose this change (required)

#577

How Has This Been Tested?

Tested on local Docker for Mac with faas-swarm.

./faas-cli secret ls -g http://127.0.0.1:8081
WARNING! Communication is not secure, please consider using HTTPS. Letsencrypt.org offers free SSL/TLS certificates.

NAME
test

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I've read the CONTRIBUTION guide
  • I have signed-off my commits with git commit -s
  • I have added tests to cover my changes.
  • All new and existing tests passed.
@LucasRoesler

This comment has been minimized.

Copy link
Member

LucasRoesler commented Jan 5, 2019

I just tested this with I also tested this with the latest build of gateway (openfaas/faas@a65df47) and openfaas-incubator/openfaas-operator#70 and it worked as expected

$ ./faas-cli-darwin secret list --gateway=http://localhost:31112
test
test2

I still need to read through the code

@alexellis

This comment has been minimized.

Copy link
Member

alexellis commented Jan 5, 2019

Should we have a heading like with the other commands?

@viveksyngh

This comment has been minimized.

Copy link
Member

viveksyngh commented Jan 5, 2019

What are the other informations that we want to display here ?

@LucasRoesler

This comment has been minimized.

Copy link
Member

LucasRoesler commented Jan 5, 2019

At the moment there is no other information besides name. Right now the gateway won't expose any other information. We could add more fields in the future, once we know they are needed and supported.


//GetSecretList get secrets list
func GetSecretList(gateway string, tlsInsecure bool) ([]schema.Secret, error) {
var results []schema.Secret

This comment has been minimized.

@LucasRoesler

LucasRoesler Jan 5, 2019

Member

We probably want a "warn insecure" block here like we do in the deploy code

faas-cli/proxy/deploy.go

Lines 81 to 85 in 200359c

if warnInsecureGateway {
if (spec.RegistryAuth != "") && !strings.HasPrefix(spec.Gateway, "https") {
fmt.Println("WARNING! Communication is not secure, please consider using HTTPS. Letsencrypt.org offers free SSL/TLS certificates.")
}
}

This comment has been minimized.

@alexellis
@alexellis

This comment has been minimized.

Copy link
Member

alexellis commented Jan 5, 2019

I think we should have a heading of Name.

@alexellis

This comment has been minimized.

Copy link
Member

alexellis commented Jan 5, 2019

All the commands in the future should have some kind of --no-header flag for automation.

@LucasRoesler

This comment has been minimized.

Copy link
Member

LucasRoesler commented Jan 6, 2019

@alexellis Sorry for being dense, when you say --no-header, you mean something like docker images -q so that the "Name" header is excluded from the output, right?

@alexellis

This comment has been minimized.

Copy link
Member

alexellis commented Jan 7, 2019

Yes 👍

if err != nil {
return err
}

This comment has been minimized.

@alexellis

alexellis Jan 7, 2019

Member

Can we build up a string instead? Have a look how it's done in other commands. This will allow us to test the output.

Please also add the header of "Name"

var secretListCmd = &cobra.Command{
Use: "list",
Short: "List all secrets",
Long: `List all secrets names and metadata`,

This comment has been minimized.

@alexellis

alexellis Jan 7, 2019

Member

We have no metadata now, so omit this.

viveksyngh added some commits Jan 5, 2019

Add secret list command to faas-cli
This commit adds `faas-cli secret list` command to the faas-cli which
will be used to get secret list from the cluster.

Signed-off-by: Vivek Singh <vivekkmr45@yahoo.in>
Add header and warning for insecure gateway
Signed-off-by: Vivek Singh <vivekkmr45@yahoo.in>

@viveksyngh viveksyngh force-pushed the viveksyngh:secret_commands branch from 4af54c7 to 75d68f4 Jan 7, 2019

return nil
}

func renderSecretList(secrets []schema.Secret) string {

This comment has been minimized.

@alexellis

alexellis Jan 8, 2019

Member

Nicely done

func GetSecretList(gateway string, tlsInsecure bool) ([]schema.Secret, error) {
var results []schema.Secret

if tlsInsecure {

This comment has been minimized.

@alexellis

alexellis Jan 8, 2019

Member

I think we only show this if tlsInsecure is off? @LucasRoesler?

This comment has been minimized.

@LucasRoesler

LucasRoesler Jan 8, 2019

Member

yeah, I think the check should be inverted here. Relatedly, the doc string on this function should document the args to make it clearer how tlsInsecure should behave

}

switch res.StatusCode {
case http.StatusOK:

This comment has been minimized.

@alexellis

alexellis Jan 8, 2019

Member

Please add http.StatusAccepted too.

This comment has been minimized.

@viveksyngh

viveksyngh Jan 8, 2019

Member

will add it. just a doubt, willGET method return statushttp.StatusAccepted?

@alexellis

This comment has been minimized.

Copy link
Member

alexellis commented Jan 8, 2019

This is looking great now Vivek. I did add one query on the tlsInsecure behavior though.. do you get a warning when you run this against http://localhost ?

@viveksyngh

This comment has been minimized.

Copy link
Member

viveksyngh commented Jan 8, 2019

I will update it, I could not get version running locally which gives secrets endpoint. @LucasRoesler
can you help me with the testing?


// secretListCmd represents the secretCreate command
var secretListCmd = &cobra.Command{
Use: "list",

This comment has been minimized.

@ivanayov

ivanayov Jan 9, 2019

Member

I think it's good to have the ls alias as well, because it's common for most commands and people are used to using it

Update insecure warning condition
This commit updates insecure warning condition and checks for
http.StausAccepted in response status.

Signed-off-by: Vivek Singh <vivekkmr45@yahoo.in>

@viveksyngh viveksyngh force-pushed the viveksyngh:secret_commands branch from accf75b to 0642cda Jan 9, 2019

Add alias for secret list command
Signed-off-by: Vivek Singh <vivekkmr45@yahoo.in>
@viveksyngh

This comment has been minimized.

Copy link
Member

viveksyngh commented Jan 9, 2019

Tested on local Docker for Mac with faas-swarm.

./faas-cli secret ls -g http://127.0.0.1:8081
WARNING! Communication is not secure, please consider using HTTPS. Letsencrypt.org offers free SSL/TLS certificates.

NAME
test
@alexellis

This comment has been minimized.

Copy link
Member

alexellis commented Jan 9, 2019

(Which commands do we want the warning for? All of them or just where we may be writing confidential data?)

@alexellis alexellis merged commit 1f9ea15 into openfaas:master Jan 10, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment