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

Strip back pipeline to Rubocop linting only #562

Merged
merged 21 commits into from
Nov 25, 2023
Merged

Conversation

Splines
Copy link
Member

@Splines Splines commented Nov 14, 2023

Despite efforts to fix the CI/CD, many things are still not working and right now we don't even care if there is a ❌ or ✅ in the checks. To do better, we get rid of our broken CI/CD completely, start fresh and will tackle all necessary things step by step. This allows us to only include things we really need right now.

This PR therefore also removes previous work GitHub workflow files even though we might need them later. But that's not a problem as they are still accessible from the commit history.

This PR does the first step by introducing a check that only passes if Ruby files changed in a PR are correctly linted according to the RuboCop rules specified in .rubocop.yml. Only the changed files are considered.

Apparently, a line-based comparison is not possible with rubocop, e.g. only check the lines that were changes in a PR, not the whole files. This shouldn't be a big problem once we file a PR that fixes Ruby style violations in the entire code base. From then on, in a pull request, style violations for files or lines should practically be the same thing.

See how a sample run looks like here. It is failing and lists all style violations nicely using GitHub workflow comments (it only took 21s for the complete check for two files). Note that if one clicks on Details next to the checks, you then have to click on Summary to see these comments presented in this way.

image

But what is even better: you will see these violations directly in the Files changed tab:

image

Note that if no Ruby files were changed in a PR, the job will pass with a ✅.

@Splines Splines self-assigned this Nov 14, 2023
@Splines Splines changed the base branch from main to mampf-next November 14, 2023 17:24

This comment was marked as off-topic.

@Splines Splines force-pushed the pipeline/backtrack branch 5 times, most recently from ce2bbee to 14caba2 Compare November 14, 2023 21:07
@Splines Splines force-pushed the pipeline/backtrack branch 4 times, most recently from a248785 to d255fc4 Compare November 14, 2023 22:28
@Splines Splines changed the title Strip back pipeline to linting only Strip back pipeline to Rubocop linting only Nov 14, 2023
@Splines Splines marked this pull request as ready for review November 14, 2023 22:51
@Splines Splines added CI/CD Continuous Integration / Continuous Delivery (aka pipeline stuff) and removed enhancement labels Nov 14, 2023
@Splines Splines mentioned this pull request Nov 14, 2023
1 task
@Splines
Copy link
Member Author

Splines commented Nov 14, 2023

Note that there will probably be situations where -- in a PR -- we find out a RuboCop cop is actually not what we want.

In that case, we could still use the admin privileges and merge without the check passing. Or better: file another PR that disables that Cop, merge it into dev and then merge dev into the feature branch.

@Splines
Copy link
Member Author

Splines commented Nov 15, 2023

Note my changes in commit 0bb85a5. See this for the severity levels. Whenever we use a severity level smaller than convention (that is info or refactor), only a warning will be shown on GitHub and the check will still pass if there are no errors.

See this run as an example for the described behavior (check passing but warning still showing). Note that "warning" is just a GitHub term here as GitHub just has these two levels (error/warning). Do not confuse with the rubocop warning severity level.

Copy link
Collaborator

@fosterfarrell9 fosterfarrell9 left a comment

Choose a reason for hiding this comment

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

LGTM, but I am amfraid that I am not really competent in this area. Maybe @christian-heusel or @henrixapp can offer some more substantial feedback.

@henrixapp
Copy link
Collaborator

Hi,

I don't think that this is the right approach. Mainly following the arguments in my comments on #441 (comment) .

Yes, the pipeline is failing, but only partially. Recent runs like this show that there are two types of errors:

  • Mistakes in the setup (the server returns a 500)
  • Buttons not being clickable, changing labels

Yes, you can go ahead and remove the pipelines, but there are better ways of solving this issue. Deleting the pipeline just removes the checkbox/reminder to fix the setup, not the problem behind the situation.

I think it is worth to invest some time in fixing the errors in cypress, by generalizing the tests and make them more user centered, less picking html-ids. Furthermore, I will happily assist you in finding better ways of checking eslint errors.

TLDR: Changing the rubocop is fine! Replacing eslint is probably long over due. Deleting the cypress pipeline would only mitigate the symptoms.

@Splines
Copy link
Member Author

Splines commented Nov 19, 2023

Hey @henrixapp, thanks a lot for your comment and your offer to help, appreciate it!

Deleting the pipeline just removes the checkbox/reminder to fix the setup, not the problem behind the situation.

Of course, that's true. However, pragmatically speaking, we cannot fix everything at the same time and need to resolve this step by step and I want to do each step as best as possible. This means it takes time while we still have to focus on other MaMpf-related things that might have higher priority, e.g. shipping the new annotation tool #533, getting next releases ready, fixing bugs etc. The reality right now is that - as far as I know - no one looks at any check at all, even if they are failing. We just merge without worrying. The goal is to really take the checks seriously. Whenever any check fails, we should be alarmed and see where the check failed. But this is not the reality right now.

Deleting the cypress pipeline would only mitigate the symptoms.

Sure and that's even advantageous in this case. After merging this PR, we will only have Rubocop linting checks for some time and can start to gain trust in the pipeline again (ok that might sound cheesy but I think that's a real thing, likewise for ad banners that one learns to fully ignore in the web, or install an adblocker but I get off track...).

While only dealing with the symptoms, it doesn't mean we well toss away the underlying issues in our mind. For the linting stuff, see this roadmap. For the CI/CD stuff I should also create such an issue and will do it soon.

there are two types of errors:

Due to the mentioned reasons, I'd like to outsource the discussion about specific bugs not related to Rubocop to future PRs that deal with the respective setup or to the issue section. I will come back to you when failing in trying to fix other parts of the pipeline ;)

@Splines Splines merged commit 943a7a4 into mampf-next Nov 25, 2023
2 checks passed
@Splines Splines deleted the pipeline/backtrack branch November 25, 2023 16:51
Splines added a commit that referenced this pull request Jan 6, 2024
* Only lint Rubocop for now (and only modified files)

deleted files are excluded

* Scope action for PRs

* Disable tests for now

* Run command in one line

* Fix RuboCop command

* Further improve linting setup

* Simplify linter setup

* Print changed ruby files

* Fix further errors and improve workflow file

* Change random Rails files to see effects

* Fix spaces in changed files

* Include target branch lookup

* Remove unnecessary ${{}} syntax

* Fetch target branch

* Improve commit retrieval

* Redo whole workflow

* Revert dummy changes in Rails files

* Use checkout@v4 (instead of @V3)

* Explicitly set fail level & force excluding files

See command line flags here:
https://docs.rubocop.org/rubocop/usage/basic_usage.html#command-line-flags

* Add more events to trigger workflow

See the docs here:
https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#pull_request

* Add printout to display RuboCop version used
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/CD Continuous Integration / Continuous Delivery (aka pipeline stuff)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants