Skip to content

Switch CI to new metadata collection #7298

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

Merged
merged 3 commits into from
Jul 28, 2021

Conversation

flip1995
Copy link
Member

@flip1995 flip1995 commented May 31, 2021

r? @xFrednet

Things we have to keep in mind:

  • This removes the template files and the scripts used for deployment from the checkout. This was added in Deploy time travel #5517. I don't think we ever needed those there. Not sure though.
  • As a result, we can't remove the python scripts yet. We have to wait until this hits a stable Clippy release. I'll just break the next stable deploy and do it by hand once.
  • This should be merged together with Adapting the lint list to Clippy's new metadata format #7279. Me and @xFrednet will coordinate the switch
  • ...?

I still have to try out some things:

  • Is it worth caching? Yes
  • Is it worth to do a release build? Nope
  • Does it actually work? With a few changes, yes
  • ...?

changelog: Clippy now uses a lint to generate its documentation 🎉

Copy link
Member

@xFrednet xFrednet left a comment

Choose a reason for hiding this comment

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

The deployment script has some special handling for tags and the beta deployment (if I understand it correctly). In #7279 we've discussed that a stable branch should be added to support the View Code link works for all website paths. I believe that should be added to the deployment script as well.

The rest looks straight forward and should hopefully work. You mentioned that you've wanted to document the CI a bit better. Do you want to do that as part of this PR or after everything works?

Thank you for taking over the CI work :)


Another question that I had recently that is slightly related: The Clippy CI currently starts on every push even in forks or it at least does for me xFrednet/rust-clippy/actions. Is that intended behavior? I'm fine with it eating up my CI time, but other users might not be in favor of it.

@flip1995
Copy link
Member Author

flip1995 commented Jun 1, 2021

In #7279 we've discussed that a stable branch should be added to support the View Code link works for all website paths. I believe that should be added to the deployment script as well.

Quite the opposite. The stable branch should not be deployed, since it would just duplicate the deployed tag. The stable branch will just be a dummy branch pushed at every release together with the tag.

Do you want to do that as part of this PR or after everything works?

Thanks for reminding / forcing me to do this. I want to do this separately. In this PR I only want the really necessary things, so if something goes wrong, we can just revert one commit and everything is fine again.

Another question that I had recently that is slightly related: The Clippy CI currently starts on every push even in forks or it at least does for me xFrednet/rust-clippy/actions. Is that intended behavior?

Yes, this only applies for the clippy.yml workflow. I think @matthiaskrgr complained when I removed the automatic builds on forks. It is just one job that runs, so it shouldn't eat that much of CI time. People probably won't even notice.

@xFrednet
Copy link
Member

xFrednet commented Jun 1, 2021

Quite the opposite. The stable branch should not be deployed, since it would just duplicate the deployed tag. The stable branch will just be a dummy branch pushed at every release together with the tag.

Yes, that's what I meant.

Thanks for reminding / forcing me to do this. I want to do this separately. In this PR I only want the really necessary things, so if something goes wrong, we can just revert one commit and everything is fine again.

Sure, makes sense. I remember reading the documentation one or two times before my first contribution to not ask any stupid questions. And it stayed in my focus since then to help other newcomers as well 🙃

Yes, this only applies for the clippy.yml workflow. I think @matthiaskrgr complained when I removed the automatic builds on forks. It is just one job that runs, so it shouldn't eat that much of CI time. People probably won't even notice.

Okay, thanks for confirming 👍

@flip1995 flip1995 force-pushed the ci-switch-to-monster branch from 97d1f7f to 76a06e5 Compare June 4, 2021 15:24
@flip1995
Copy link
Member Author

flip1995 commented Jun 4, 2021

This deploy workflow run won't actually work. For that I have to reopen this PR from a branch directly in this repo.

@flip1995
Copy link
Member Author

flip1995 commented Jun 4, 2021

@xFrednet I think I addressed everything we discussed in your previous review. Before I push it to a branch in this repo for testing, can you take another look, if you still notice things that might need another fix?

Copy link
Member

@xFrednet xFrednet left a comment

Choose a reason for hiding this comment

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

The rest looks ready for a test IMO 👍

Is it intended, that the CI is red in this PR?

@flip1995
Copy link
Member Author

flip1995 commented Jun 5, 2021

Is it intended, that the CI is read in this PR?

I guess you mean "red"? Or do you mean that it is run? In the first case not intended, but expected, in the second, yes.

@xFrednet
Copy link
Member

xFrednet commented Jun 5, 2021

I guess you mean "red"?

Yes 😅

Everything should be covered in that case :)

@flip1995 flip1995 force-pushed the ci-switch-to-monster branch from 76a06e5 to 77187c9 Compare June 25, 2021 15:25
@flip1995 flip1995 mentioned this pull request Jun 25, 2021
@flip1995 flip1995 force-pushed the ci-switch-to-monster branch from 77187c9 to 6390275 Compare June 25, 2021 16:54
@flip1995
Copy link
Member Author

Alright, I tested everything in #7401. So we should be ready for the move, after I squashed the commits.

I won't get to do this this weekend, but plan to do it next Saturday 2021-07-03

@xFrednet
Copy link
Member

Awesome!!! I'll review this PR again and also look at the test. Next weekend also works for me, I'll rebase #7279 and create a new PR (shortly before the switch) to update the documentation to use hashtags instead of bold text. Is there anything else you need help with? 🙃

@flip1995
Copy link
Member Author

Is there anything else you need help with?

Nope, I think I'm good 👍

@flip1995 flip1995 added the S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) label Jul 14, 2021
@flip1995 flip1995 self-assigned this Jul 15, 2021
@xFrednet xFrednet mentioned this pull request Jul 28, 2021
1 task
@flip1995 flip1995 force-pushed the ci-switch-to-monster branch from 6390275 to 5d412c0 Compare July 28, 2021 12:08
@flip1995
Copy link
Member Author

* As a result, we can't remove the python scripts yet. We have to wait until this hits a stable Clippy release.

This isn't the case anymore, since I decided to break the stable deploy for one cycle and do it by hand instead.

flip1995 added 2 commits July 28, 2021 14:16
This updates all the deploy scripts and the deploy workflow.

The deploy workflow now runs the metadata collector to collect the lint
documentation. It also changes the files that are checked out in the
deploy workflow from master and adds an explanation why we have to do
this.
@flip1995 flip1995 force-pushed the ci-switch-to-monster branch from 5d412c0 to fe25282 Compare July 28, 2021 12:20
@flip1995 flip1995 changed the title WIP: Switch CI to new metadata collection Switch CI to new metadata collection Jul 28, 2021
@flip1995 flip1995 marked this pull request as ready for review July 28, 2021 12:21
@flip1995
Copy link
Member Author

@bors r=xFrednet,flip1995 rollup

@bors
Copy link
Contributor

bors commented Jul 28, 2021

📌 Commit fe25282 has been approved by xFrednet,flip1995

@bors
Copy link
Contributor

bors commented Jul 28, 2021

🌲 The tree is currently closed for pull requests below priority 1000. This pull request will be tested once the tree is reopened.

flip1995 added a commit to flip1995/rust-clippy that referenced this pull request Jul 28, 2021
…ednet,flip1995

Switch CI to new metadata collection

r? `@xFrednet`

Things we have to keep in mind:

- This removes the template files and the scripts used for deployment from the checkout. This was added in rust-lang#5517. I don't think we ever needed those there. Not sure though.
- ~~As a result, we can't remove the python scripts yet. We have to wait until this hits a stable Clippy release.~~ I'll just break the next stable deploy and do it by hand once.
- This should be merged together with rust-lang#7279. Me and `@xFrednet` will coordinate the switch
- ...?

I still have to try out some things:

- [x] Is it worth caching? Yes
- [x] ~~Is it worth to do a release build?~~ Nope
- [x] Does it actually work? With a few changes, yes
- [ ] ...?

changelog: Clippy now uses a lint to generate its documentation 🎉
@flip1995
Copy link
Member Author

@bors r=xFrednet,flip1995 rollup

@bors
Copy link
Contributor

bors commented Jul 28, 2021

📌 Commit c951a3c has been approved by xFrednet,flip1995

@bors
Copy link
Contributor

bors commented Jul 28, 2021

🌲 The tree is currently closed for pull requests below priority 1000. This pull request will be tested once the tree is reopened.

bors added a commit that referenced this pull request Jul 28, 2021
Rollup of 3 pull requests

Successful merges:

 - #7279 (Adapting the lint list to Clippy's new metadata format)
 - #7298 (Switch CI to new metadata collection)
 - #7420 (Update lint documentation to use markdown headlines)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup

changelog: rollup
@bors bors merged commit bcdf147 into rust-lang:master Jul 28, 2021
bors added a commit that referenced this pull request Jul 29, 2021
Updated changelog for 1.55

This has again been a bit of work, but I'm happy to notice that my English is still improving, and I'm getting faster at these things. That's a very nice side effect of contributing and getting feedback on reviews 😊

Moving on, there were a few things that I was unsure about:
* The PR rust-lang/rust#86717 changes an old entry in the change log, is this worth mentioning? I've left it out for now.
* The stabilization of `cargo clippy --fix` is quite awesome and important IMO. It sadly gets a bit lost in the *Other* entry, as it's the last one. Do we maybe want to move it somewhere else or change the headline order for this release?
* I've listed the introduction of the new `suspicious` group under the *Moves and Deprecations* section. Is this alright, or should it be moved to the *Other* section as well?
* Last but definitely not least, some fun! I've used the 🎉 emoji in the `cargo clippy --fix` entry, is this okay?

Sorry for the bombardment of questions xD

---

The PR already includes the entries for the new metadata collection and website updates. These are not merged yet, but should probably be to make this correct. This might also require the commit hashes to be updated (Not sure on this, though). It would actually be super fitting to get this into this release as we also stabilize `--fix`. TODOs:
* [x] Merge metadata collection PRs:
  1. #7279
  2. #7298
  3. #7420 (Hope to not get any merge conflicts)

---

[Rendered 📰](https://github.com/xFrednet/rust-clippy/blob/changelog-1-55/CHANGELOG.md)

r? `@flip1995`

changelog: none
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants