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

Don't skip compliance on docs only PRs #24017

Closed
wants to merge 3 commits into from

Conversation

yardenshoham
Copy link
Member

We should make sure the docs linting happens

We should make sure the docs linting happens
@yardenshoham yardenshoham added topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. labels Apr 9, 2023
@yardenshoham yardenshoham added this to the 1.20.0 milestone Apr 9, 2023
@delvh
Copy link
Member

delvh commented Apr 9, 2023

Hmm… Except that seems like a lot of unnecessary CI overhead.
What about splitting compliance into compliance-backend, compliance-frontend, and compliance-docs?
That will probably work the best, and even offer future CI optimizations.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 9, 2023
@silverwind
Copy link
Member

silverwind commented Apr 9, 2023

What about splitting compliance into compliance-backend, compliance-frontend, and compliance-docs?

Will probably make it a lot slower because every one these pipelines needs to re-install dependencies because drone pipelines share nothing. I dislike introducing more pipelines because of this issue.

Copy link
Member

@silverwind silverwind left a comment

Choose a reason for hiding this comment

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

We don't have so many docs-only PRs, so this is fine imho.

Could introduce a optimized docs-only pipeline later that does the markdownlint only.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Apr 9, 2023
@silverwind
Copy link
Member

silverwind commented Apr 9, 2023

Such a docs-only pipeline could consist of single npx markdownlint docs *.md command. With node_modules absent, npx will install into global cache. The only issue with this is that it would use the latest version instead of the one in package.json. That could be done via something like this:

npx markdownlint-cli@$(jq -r '.devDependencies["markdownlint-cli"]' package.json) docs *.md

@yardenshoham
Copy link
Member Author

Would you like this in this PR or a future one?

@silverwind
Copy link
Member

Let's do later. It depends on jq which is currently absent in the test-env:

https://gitea.com/gitea/test-env/pulls/22

@yardenshoham yardenshoham requested a review from lunny April 9, 2023 10:05
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Apr 9, 2023
@yardenshoham yardenshoham added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Apr 9, 2023
@lunny
Copy link
Member

lunny commented Apr 9, 2023

No, I object to this change. I think we should have a separate pipeline for docs.
It's a chicken and egg problem. Why do we have so few docs-only PRs? Because CI took too long for every change of a docs-only PR which prevents people have an interesting to do that. This PR will make docs-only PR have to compile the source which is necessary.

@yardenshoham yardenshoham removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Apr 9, 2023
@yardenshoham
Copy link
Member Author

Ok. I'll try again after https://gitea.com/gitea/test-env/pulls/22 lands. Just know that until then, we will push docs code to main without linting

@yardenshoham yardenshoham removed this from the 1.20.0 milestone Apr 9, 2023
@silverwind
Copy link
Member

I guess a node_modules installation may acceptable in that pipeline, it only takes 15 seconds, and in such a case we don't need jq. I'll try adding that now.

@silverwind
Copy link
Member

#24021 for that.

@go-gitea go-gitea locked as resolved and limited conversation to collaborators Jul 31, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants