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

GCP CCS command #192

Merged
merged 1 commit into from
Nov 30, 2020
Merged

GCP CCS command #192

merged 1 commit into from
Nov 30, 2020

Conversation

igoihman
Copy link

No description provided.

@@ -75,6 +80,12 @@ func init() {

arguments.AddProviderFlag(fs, &args.provider)
arguments.AddCCSFlags(fs, &args.ccs)

fs.Var(
&args.gcpServiceAccount,
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally declaring this flag would be part of arguments.AddCCSFlags().
One way to do that that might be move the FilePath var inside CCS struct, and making constructGCPCredentials a method on CCS struct. Don't know if worth it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Opened #199 for some validations we should improve.

@cben
Copy link
Contributor

cben commented Nov 30, 2020

Looks great 👍

@igoihman igoihman changed the title [WIP] GCP CCS command GCP CCS command Nov 30, 2020
@igoihman igoihman merged commit c7e21f0 into openshift-online:master Nov 30, 2020
@cben cben mentioned this pull request Dec 3, 2020
4 tasks

fs.Var(
&args.gcpServiceAccount,
"service-account-file",
Copy link
Contributor

Choose a reason for hiding this comment

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

WDYT about renaming this --gcp-service-account-file? 4 more characters to type but maybe worth it to keep the flags organized.

(BTW, maybe it's time to start using Cobra's flag groups to organize the help 🤔)

@@ -26,14 +26,33 @@ import (
"reflect"
"strings"

"github.com/mattn/go-isatty"
isatty "github.com/onsi/ginkgo/reporters/stenographer/support/go-isatty"
Copy link
Contributor

Choose a reason for hiding this comment

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

@igoihman was this deliberate?
https://github.com/onsi/ginkgo/tree/master/reporters/stenographer/support/go-isatty/
looks like an old, less maintained, copy of upstream https://github.com/mattn/go-isatty/.
Or was this simply replaced by IDE being too clever?

Copy link
Contributor

Choose a reason for hiding this comment

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

=> #201 if accidental.

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.

2 participants