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

v2: Remove need for ONESHELL in go lint #4535

Merged
merged 1 commit into from
Dec 27, 2022
Merged

v2: Remove need for ONESHELL in go lint #4535

merged 1 commit into from
Dec 27, 2022

Conversation

ukclivecox
Copy link
Contributor

Removes the need to use the ONESHELL directive in Makefiles for Go lint download. ONESHELL is a global directive for the entire Makefile and causes issues in getting the go license updates to run as it stops normal error failure during Makefile runs, i.e. an error code from a command should stop the Make run.

From docs:

Even with this special feature, however, makefiles with .ONESHELL will behave differently in ways that could be noticeable. For example, normally if any line in the recipe fails, that causes the rule to fail and no more recipe lines are processed. Under .ONESHELL a failure of any but the final recipe line will not be noticed by make. You can modify .SHELLFLAGS to add the -e option to the shell which will cause any failure anywhere in the command line to cause the shell to fail, but this could itself cause your recipe to behave differently. Ultimately you may need to harden your recipe lines to allow them to work with .ONESHELL.

if ! $${installed}; then
curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh \
${.GOLANGCILINT_PATH}/golangci-lint:
curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh \
Copy link
Member

Choose a reason for hiding this comment

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

Are we always installing go lint every time now? If so how long does it usually take?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are not. It should only install if its not there.

Copy link
Member

Choose a reason for hiding this comment

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

It is not clear to me how this is done given that we are removing the logic to check if the location of golint exist?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

${.GOLANGCILINT_PATH}/golangci-lint: should only run if needed.

Copy link
Member

@sakoush sakoush left a comment

Choose a reason for hiding this comment

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

LGTM

@ukclivecox ukclivecox merged commit ab11a49 into SeldonIO:v2 Dec 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants