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

tekton task to run linting #954

Merged
merged 2 commits into from
Jun 13, 2019
Merged

Conversation

vdemeester
Copy link
Member

Changes

This adds a Task to run golangci-lint on the repository.
This is part of the dogfooding effort on tektoncd/pipeline.

Closes #558

cc @bobcatfish
cc @chmouel I know you'll like it 👼

Signed-off-by: Vincent Demeester [email protected]

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

See the contribution guide
for more details.

@googlebot googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Jun 5, 2019
@tekton-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vdemeester

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

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 5, 2019
@tekton-robot tekton-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 5, 2019
@vdemeester
Copy link
Member Author

/hold

@tekton-robot tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 5, 2019
@chmouel
Copy link
Member

chmouel commented Jun 5, 2019

nice one, we may need to tweaks things with a golangci.yaml file in the future

- name: flags
description: flags to use for the test command
- name: version
default: golangci-lint version to use

Choose a reason for hiding this comment

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

Should be "description", not default.

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed 🤦‍♂️

Copy link
Collaborator

Choose a reason for hiding this comment

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

bump :D

Copy link

@houshengbo houshengbo left a comment

Choose a reason for hiding this comment

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

One necessary change needed. @vdemeester

@dlorenc
Copy link
Contributor

dlorenc commented Jun 6, 2019

It looks like you want this to live in catalog as well, should it just go straight there?

@vdemeester
Copy link
Member Author

@dlorenc yeah I started working on those (this PR and #949) but I feel it's better to have them in the catalog in a slightly more generic way 👼 (and just "apply" those on the test cluster at first)

@vdemeester vdemeester mentioned this pull request Jun 7, 2019
2 tasks
@vdemeester
Copy link
Member Author

@dlorenc let's merge this like the unit test one for now, and we'll refine this from tektoncd/catalog#43 and #964

Copy link
Collaborator

@bobcatfish bobcatfish left a comment

Choose a reason for hiding this comment

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

🎉 amazing! 🎉

Would you mind adding something to https://github.com/tektoncd/pipeline/tree/master/tekton#tekton-repo-cicd about this? 😇 (Feel free to delete stuff that's already in that doc if it makes sense, I just think it makes sense to have something documenting these Tasks we're adding for the repo and how to run them)

description: package (and its children) under test
default: github.com/tektoncd/pipeline
- name: flags
description: flags to use for the test command
Copy link
Collaborator

Choose a reason for hiding this comment

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

i think this description might not be quite right? ("for the lint command"?)

Copy link
Member Author

Choose a reason for hiding this comment

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

🤦‍♂️

@vdemeester vdemeester force-pushed the 558-linting-task branch 2 times, most recently from a259af0 to e262f4e Compare June 11, 2019 15:21
[`TaskRuns`](https://github.com/tektoncd/pipeline/blob/master/docs/taskruns.md)
and
[`PipelineResources`](https://github.com/tektoncd/pipeline/blob/master/docs/resources.md).

Copy link
Collaborator

Choose a reason for hiding this comment

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

ty ty! 🎉

inputs:
params:
- name: flags
value: --verbose
Copy link
Collaborator

Choose a reason for hiding this comment

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

😇 since this is the default we probably don't need it anymore? 😇

This adds a Task to run `golangci-lint` on the repository.
This is part of the dogfooding effort on tektoncd/pipeline.

Signed-off-by: Vincent Demeester <[email protected]>
This is a work-in-progress, but let's start documenting what are those
tasks and what are they used for.

Signed-off-by: Vincent Demeester <[email protected]>
Copy link
Collaborator

@bobcatfish bobcatfish left a comment

Choose a reason for hiding this comment

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

/lgtm
/meow space

@tekton-robot
Copy link
Collaborator

@bobcatfish: cat image

In response to this:

/lgtm
/meow space

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 11, 2019
@vdemeester
Copy link
Member Author

/hold cancel

@tekton-robot tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 13, 2019
@vdemeester vdemeester added this to the Pipelines 0.5 🐱 milestone Jun 13, 2019
@vdemeester
Copy link
Member Author

/test pull-tekton-pipeline-build-tests

@tekton-robot tekton-robot merged commit 64ad2b7 into tektoncd:master Jun 13, 2019
@vdemeester vdemeester deleted the 558-linting-task branch June 13, 2019 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create Task for running linting (markdownlint, …)
7 participants