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

mc: Inherit global flags for initialization messages. #2189

Merged
merged 1 commit into from
Jun 26, 2017

Conversation

harshavardhana
Copy link
Member

Fixes #2089

@codecov-io
Copy link

codecov-io commented Jun 21, 2017

Codecov Report

Merging #2189 into master will decrease coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #2189      +/-   ##
=========================================
- Coverage    9.12%   9.12%   -0.01%     
=========================================
  Files          93      93              
  Lines        6902    6906       +4     
=========================================
  Hits          630     630              
- Misses       6142    6146       +4     
  Partials      130     130
Impacted Files Coverage Δ
cmd/share.go 0% <0%> (ø) ⬆️
cmd/main.go 0% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d01a50d...e4d8f4a. Read the comment docs.

cmd/main.go Outdated
@@ -174,14 +175,31 @@ func getSystemData() map[string]string {
}
}

type initMessage struct {
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a comment here?

cmd/main.go Outdated
}

func (m initMessage) String() string {
return m.Message
Copy link
Member

Choose a reason for hiding this comment

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

Missing some colors here (it was green)

Copy link
Member

@vadmeste vadmeste left a comment

Choose a reason for hiding this comment

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

Some minors changes, LGTM otherwise.

@harshavardhana
Copy link
Member Author

Fixed all the comments @vadmeste

vadmeste
vadmeste previously approved these changes Jun 23, 2017
Copy link
Member

@vadmeste vadmeste left a comment

Choose a reason for hiding this comment

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

LGTM & tested

Copy link
Contributor

@krishnasrinivas krishnasrinivas left a comment

Choose a reason for hiding this comment

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

krishna@escape:~$ mc --no-color --quiet --json --config-folder=/tmp/cfg config host list
{"status":"success","msg":"Configuration written to `/tmp/cfg/config.json`. Please update your access credentials."}
{"status":"success","msg":"Successfully created `/tmp/cfg/share`."}
{"status":"success","msg":"Initialized share uploads `/tmp/cfg/share/uploads.json` file."}
{"status":"success","msg":"Initialized share downloads `/tmp/cfg/share/downloads.json` file."}
{"status":"success","alias":"gcs  :","URL":"https://storage.googleapis.com","accessKey":"YOUR-ACCESS-KEY-HERE","secretKey":"YOUR-SECRET-KEY-HERE","api":"S3v2"}
{"status":"success","alias":"local:","URL":"http://localhost:9000"}
{"status":"success","alias":"play :","URL":"https://play.minio.io:9000","accessKey":"Q3AM3UQ867SPQQA43P2F","secretKey":"zuf+tfteSlswRu7BJ86wekitnifILbZam1KYY3TG","api":"S3v4"}
{"status":"success","alias":"s3   :","URL":"https://s3.amazonaws.com","accessKey":"YOUR-ACCESS-KEY-HERE","secretKey":"YOUR-SECRET-KEY-HERE","api":"S3v4"}
krishna@escape:~$ 

With this output, since we don't have a type field it will be tough for the parsers to know if it is an init message or a config-list message. Better not to intermix messages of different types? If we want to intermix then the message should contain a type field for the parsers to differentiate.

@harshavardhana
Copy link
Member Author

With this output, since we don't have a type field it will be tough for the parsers to know if it is an init message or a config-list message. Better not to intermix messages of different types? If we want to intermix then the message should contain a type field for the parsers to differentiate.

Json is free from those problems if the relevant fields don't exist they just are nil. So it is not a big deal this is just for consistency. Ideally this case is never hit and it is usually never parsed in this manner.

@vadmeste
Copy link
Member

I see it is better if we don't print init related messages when json is activated. It doesn't seem that someone will need that. Besides as @krishnasrinivas said, it intermixes json structure.

@harshavardhana
Copy link
Member Author

I see it is better if we don't print init related messages when json is activated. It doesn't seem that someone will need that. Besides as @krishnasrinivas said, it intermixes json structure.

Made the change check again @vadmeste @krishnasrinivas - please!

Copy link
Member

@vadmeste vadmeste left a comment

Choose a reason for hiding this comment

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

LGTM and tested

@harshavardhana harshavardhana merged commit 05f588f into minio:master Jun 26, 2017
@harshavardhana harshavardhana deleted the json branch June 26, 2017 16:10
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.

4 participants