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

Separate lint into own job #21326

Merged
merged 11 commits into from Oct 8, 2020
Merged

Separate lint into own job #21326

merged 11 commits into from Oct 8, 2020

Conversation

Aerijo
Copy link
Member

@Aerijo Aerijo commented Sep 16, 2020

Requirements for Adding, Changing, or Removing a Feature

Description of the Change

Separates the lint step into its own job.

Linting works on the source code, is not platform dependent, and does not require a lot of installations (i.e., not apm). This makes it ideal to run on a single job separately from the actual builds to get much faster lint feedback.

Alternate Designs

Have linting as part of the steps in a single build (e.g., the Linux one). This should not affect the lint result, but does not offer the quicker feedback a separate job might. Currently, the bootstrap task responsible for installing script dependencies (necessary for linter) also installs apm and all of Atom's dependencies, which aren't necessary to start linting.

  • It might be possible to add linting as part of the bootstrap task then. But that design would be weird.

Possible Drawbacks

  • An extra job might slow down the start of the other jobs.

    • On the contrary, it appears to run fine and a significant amount is run when the GetReleaseVersion task is running
  • I think the current design will only lint PRs now.

    • Should now be fixed to lint and be added as a dependency everywhere the platform templates are used.

Verification Process

See how CI looks.

Release Notes

NA

@sadick254
Copy link
Contributor

sadick254 commented Sep 16, 2020

I have been thinking of doing this. Thanks @Aerijo for this PR.

@Aerijo
Copy link
Member Author

Aerijo commented Sep 16, 2020

Looks like Lint can run alongside the GetReleaseVersion job, which is nice. I quite like the 2 minute turnaround time.

  • Does anyone know if this can benefit from caching? Unfortunately it installs more than just the lint dependencies, including some unrelated native modules that need to be compiled.

    • Caching is non trivial because the existing template manages three caches and errors if the unused ones aren't available at the end. The job is fast enough as is.
  • Could the original linting be left as part of the build jobs, and just disabled if the Lint job will be run / has started? This prevents oversights in forgetting to lint.

script/vsts/lint.yml Outdated Show resolved Hide resolved
aminya
aminya approved these changes Sep 17, 2020
Copy link
Contributor

@aminya aminya left a comment

This is a very good improvement.

@aminya
Copy link
Contributor

aminya commented Sep 17, 2020

Should we stop running the actual CI if the lint fails?

@Aerijo
Copy link
Member Author

Aerijo commented Sep 17, 2020

Should we stop running the actual CI if the lint fails?

I don't think it's necessary. Any tests failures may still be relevant, and if the author pushes changes before the tests finish then the old run will be cancelled anyway.

script/vsts/lint.yml Outdated Show resolved Hide resolved
@Aerijo
Copy link
Member Author

Aerijo commented Sep 17, 2020

I've been trying to reproduce the find-and-replace flaky test failures locally and on the package repo CI, but so far nothing. atom/find-and-replace#1133

Copy link
Contributor

@aminya aminya left a comment

I made a tool to install only lint dependencies. Please merge that PR and apply the changes I suggested.
Aerijo#1

script/vsts/lint.yml Show resolved Hide resolved
@aminya aminya mentioned this pull request Sep 17, 2020
@darangi darangi merged commit a60471b into atom:master Oct 8, 2020
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants