Skip to content

linting: use local scripts to remove sudo need#1831

Merged
georglauterbach merged 1 commit intodocker-mailserver:masterfrom
ap-wtioit:master-install_linters_without_sudo
Feb 23, 2021
Merged

linting: use local scripts to remove sudo need#1831
georglauterbach merged 1 commit intodocker-mailserver:masterfrom
ap-wtioit:master-install_linters_without_sudo

Conversation

@ap-wtioit
Copy link
Copy Markdown
Contributor

@ap-wtioit ap-wtioit commented Feb 23, 2021

Description

this change installs the linting tools in a local tools directory. this removes the need for sudo curl and replacing binaries in /usr/local/bin and /usr/bin.

Issues with the previous code:

  • /usr/local/bin and /usr/bin might be shared on CI systems by multiple builds influencing each other
  • the old code might leave some of the tools shared with other build unexecutable when downloading them to /usr/local/bin and /usr/bin for short periods of time (breaking other builds)

Type of change

  • Improvement (non-breaking change that does improve existing functionality)

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • New and existing unit tests pass locally with my changes

Info @wt-io-it

@ap-wtioit
linting: use local scripts to remove sudo need
9df60f6 
install linting tools locally to remove the need for
sudo curling scripts and installing them for all users
@georglauterbach
Copy link
Copy Markdown
Member

georglauterbach commented Feb 23, 2021

I like this change.

LGTM

@georglauterbach georglauterbach added area/ci area/enhancement area/scripts kind/improvement Improve an existing feature, configuration file or the documentation labels Feb 23, 2021
wernerfred
wernerfred reviewed Feb 23, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@wernerfred wernerfred left a comment

Choose a reason for hiding this comment

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

A few thoughts from my side:

Imho there is a problem when "installing" to a local path in the repository as one might run these linting checks locally to test befor opening a PR. Therefore the local repository would include the tools as a local change. There are multiple possibilities to solve this. For example make it optional to remove them afterwards via make clean or to add tools/** to .gitignore.

For a long sight: I am preparing a PR (which I came up with already before the migration started) to switch the linting tools etc to their docker versions. This will solve the issue with "installing" completely and should not be a problem for anyone as everyone is using docker already when using docker-mailserver.
But for the meantime i like your change and will approve if the issue mentioned above is resolved/discussed.

@ap-wtioit
Copy link
Copy Markdown
Contributor Author

ap-wtioit commented Feb 23, 2021

@wernerfred please look at the Files changed tab, tools is already added to .gitignore

@georglauterbach
Copy link
Copy Markdown
Member

georglauterbach commented Feb 23, 2021

There are multiple possibilities to solve this. For example make it optional to remove them afterwards via make clean or to add tools/** to .gitignore.

Did I miss something?

#################################################
###  Linting Tools  #############################
#################################################

tools/

?

@wernerfred
Copy link
Copy Markdown
Member

wernerfred commented Feb 23, 2021

omg, my bad. Looked at the changed files of course but (howsoever) must have overlooked changes to .gitignore

@georglauterbach georglauterbach merged commit b4b4538 into docker-mailserver:master Feb 23, 2021
@casperklein casperklein mentioned this pull request Feb 23, 2021
Merged
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wernerfred wernerfred wernerfred approved these changes

@georglauterbach georglauterbach georglauterbach approved these changes

@casperklein casperklein Awaiting requested review from casperklein

Assignees

@ap-wtioit ap-wtioit

Labels

area/ci area/scripts kind/improvement Improve an existing feature, configuration file or the documentation

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ap-wtioit @georglauterbach @wernerfred