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

Improve github action #493

Merged
merged 2 commits into from Feb 6, 2020
Merged

Conversation

@ferhatelmas
Copy link
Contributor

ferhatelmas commented Feb 4, 2020

  • whitespace (trim and indentation)
  • step naming
  • drop id from setup

Related to the comment: #491 (comment)

Own initiative, feel free to close abruptly.

@ahmetb

This comment has been minimized.

Copy link
Member

ahmetb commented Feb 4, 2020

Can we not do the indentation? I find unnecessary yaml indents superfluous. :)

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Feb 4, 2020

Codecov Report

Merging #493 into master will increase coverage by 0.52%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #493      +/-   ##
========================================
+ Coverage   56.47%    57%   +0.52%     
========================================
  Files          19     21       +2     
  Lines         926    956      +30     
========================================
+ Hits          523    545      +22     
- Misses        350    357       +7     
- Partials       53     54       +1
Impacted Files Coverage Δ
cmd/krew/cmd/internal/setup_check.go 91.66% <0%> (ø)
cmd/krew/cmd/internal/security_notice.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 4303b6a...c04ddb8. Read the comment docs.

@ferhatelmas

This comment has been minimized.

Copy link
Contributor Author

ferhatelmas commented Feb 4, 2020

Can we not do the indentation? I find unnecessary yaml indents superfluous. :)

As you wish.

In general, I setup lint in my environment.

@ahmetb

This comment has been minimized.

Copy link
Member

ahmetb commented Feb 4, 2020

YAML indentation is not done in Kubernetes ecosystem, in general so avoiding would be the best.

* whitespace (trim)
* better step naming
@ferhatelmas ferhatelmas force-pushed the ferhatelmas:improve-action branch from 61bbe53 to a9c5c56 Feb 5, 2020
@k8s-ci-robot k8s-ci-robot added size/M and removed size/L labels Feb 5, 2020
@ferhatelmas

This comment has been minimized.

Copy link
Contributor Author

ferhatelmas commented Feb 5, 2020

YAML indentation is not done in Kubernetes ecosystem, in general so avoiding would be the best.

Better to be consistent. Reverted indentation but kept the trimming.

Copy link
Contributor

corneliusweig left a comment

Overall looks good to me.

- name: Verify code quality

- name: Ensure go modul is tidy

This comment has been minimized.

Copy link
@corneliusweig

corneliusweig Feb 5, 2020

Contributor

There's a typo here.

- name: Verify code quality

- name: Ensure go modul is tidy

This comment has been minimized.

Copy link
@ahmetb

ahmetb Feb 5, 2020

Member

Ensure go.mod is already tidied

@ahmetb

This comment has been minimized.

Copy link
Member

ahmetb commented Feb 5, 2020

nits but /lgtm

@ahmetb

This comment has been minimized.

Copy link
Member

ahmetb commented Feb 6, 2020

/lgtm
/approve
Thanks!

@k8s-ci-robot

This comment has been minimized.

Copy link

k8s-ci-robot commented Feb 6, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahmetb, ferhatelmas

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit 867ad1e into kubernetes-sigs:master Feb 6, 2020
3 of 4 checks passed
3 of 4 checks passed
build
Details
tide Not mergeable. Needs approved, lgtm labels.
Details
cla/linuxfoundation ferhatelmas authorized
Details
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
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.