Skip to content
This repository has been archived by the owner on Oct 23, 2023. It is now read-only.

Go Linter Github action #283

Merged
merged 6 commits into from
Apr 12, 2022
Merged

Go Linter Github action #283

merged 6 commits into from
Apr 12, 2022

Conversation

yk-x-25
Copy link
Contributor

@yk-x-25 yk-x-25 commented Apr 10, 2022

Signed-off-by: Yukesh Kumar [email protected]

TL;DR

Adds go linter github actions

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Complete description

  • makes use of golangci-lint in the github actions to check for linter issues.

Tracking Issue

#1874

fixes https://github.com/flyteorg/flyte/issues/

Follow-up issue

NA

Signed-off-by: Yukesh Kumar <[email protected]>
@welcome
Copy link

welcome bot commented Apr 10, 2022

Thank you for opening this pull request! 🙌

These tips will help get your PR across the finish line:

  • Most of the repos have a PR template; if not, fill it out to the best of your knowledge.
  • Sign off your commits (Reference: DCO Guide).

stable: 'false'
go-version: '1.16.1'

- name: Lint
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for opening a PR. I believe these checks are available in flyteorg/flytetools repo and should be referenced here as reusable workflows...
@evalsocket can you send links for examples from other repos?

Copy link
Contributor

Choose a reason for hiding this comment

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

@TheYk98 @EngHabu We need to change lint workflow here https://github.com/flyteorg/flytetools/blob/master/.github/workflows/lint.yml

Currently we didn't migrated flyteidl changes, @TheYk98 it would be great if you can use re use able workflow here like https://github.com/flyteorg/flytectl/blob/master/.github/workflows/checks.yml#L19

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lemme check this mate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed wrt comment @evalsocket @EngHabu

Signed-off-by: Yukesh Kumar <[email protected]>
@yindia
Copy link
Contributor

yindia commented Apr 11, 2022

@TheYk98 amazing, Just have few comments

After your changes now we have two workflow that run lint, Can you split the workflow https://github.com/flyteorg/flyteidl/blob/master/.github/workflows/verification.yml#L10-L29 ? Currently we have one workflow that we use for test & lint together.

For example check flytectl https://github.com/flyteorg/flytectl/blob/master/.github/workflows/checks.yml#L19-L31

@yk-x-25
Copy link
Contributor Author

yk-x-25 commented Apr 11, 2022

@TheYk98 amazing, Just have few comments

After your changes now we have two workflow that run lint, Can you split the workflow https://github.com/flyteorg/flyteidl/blob/master/.github/workflows/verification.yml#L10-L29 ? Currently we have one workflow that we use for test & lint together.

For example check flytectl https://github.com/flyteorg/flytectl/blob/master/.github/workflows/checks.yml#L19-L31

Hey @evalsocket got you . So can we just remove the existing step in the mentioned wf ?

@yindia
Copy link
Contributor

yindia commented Apr 11, 2022

You can use same workflow file for defining both re useable WF, And remove old one

Signed-off-by: Yukesh Kumar <[email protected]>
Signed-off-by: Yukesh Kumar <[email protected]>
@codecov
Copy link

codecov bot commented Apr 11, 2022

Codecov Report

❗ No coverage uploaded for pull request base (master@b1bb44f). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head 0315ffa differs from pull request most recent head 80a32d0. Consider uploading reports for the commit 80a32d0 to get more accurate results

@@            Coverage Diff            @@
##             master     #283   +/-   ##
=========================================
  Coverage          ?   74.77%           
=========================================
  Files             ?       15           
  Lines             ?      991           
  Branches          ?        0           
=========================================
  Hits              ?      741           
  Misses            ?      218           
  Partials          ?       32           

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 b1bb44f...80a32d0. Read the comment docs.

@yindia yindia enabled auto-merge (squash) April 12, 2022 09:48
@yindia yindia disabled auto-merge April 12, 2022 09:48
.github/workflows/verification.yml Show resolved Hide resolved
.github/workflows/verification.yml Show resolved Hide resolved
@yindia yindia merged commit 237f025 into flyteorg:master Apr 12, 2022
@welcome
Copy link

welcome bot commented Apr 12, 2022

Congrats on merging your first pull request! 🎉

eapolinario pushed a commit that referenced this pull request Sep 8, 2023
* Usess re-usable linter

Signed-off-by: Yukesh Kumar <[email protected]>

Co-authored-by: Yuvraj <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants