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

optimize image tag script #911

Merged
merged 2 commits into from
Aug 20, 2019
Merged

Conversation

daixiang0
Copy link
Contributor

From man git rev-parse and man head, no need to run head again.

$ git rev-parse --short=7 HEAD
ae35d2c

Also, sed should better add quota.

Signed-off-by: Xiang Dai 764524258@qq.com

@daixiang0 daixiang0 requested a review from sh0rez August 19, 2019 03:06
Copy link
Member

@sh0rez sh0rez left a comment

Choose a reason for hiding this comment

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

Hello, thanks for your contribution.

The head might be required in the future and the comment was indented that way on purpose, which means I would rather not like to have these changes merged.

tools/image-tag Outdated

# If this is a tag, use it, otherwise branch-hash
Copy link
Member

Choose a reason for hiding this comment

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

This indentation was on purpose, please leave 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.

why need it? The comment in bash script seems no one to read.

tools/image-tag Outdated
@@ -5,9 +5,9 @@ set -o nounset
set -o pipefail

WIP=$(git diff --quiet || echo '-WIP')
BRANCH=$(git rev-parse --abbrev-ref HEAD | sed s#/#-#g)
SHA=$(git rev-parse --short=7 HEAD | head -c7)
Copy link
Member

Choose a reason for hiding this comment

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

When 7 chars are not enough to be unique, git automatically uses more. We are forcing to 7 here, as we are doing for grafana/grafana as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Get it. Add comment here is better i think.
Still perfer to add quota for sed.

Signed-off-by: Xiang Dai <764524258@qq.com>
Signed-off-by: Xiang Dai <764524258@qq.com>
Copy link
Member

@rfratto rfratto left a comment

Choose a reason for hiding this comment

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

LGTM!

@rfratto rfratto merged commit 777fcfc into grafana:master Aug 20, 2019
@daixiang0 daixiang0 deleted the remove-useless-pipe branch August 21, 2019 02:20
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