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

Add actions to automatically bump man page month #3748

Merged
merged 2 commits into from Jun 25, 2020

Conversation

utkarsh2102
Copy link
Contributor

At the beginning of every month, the CI breaks as it has a check that makes sure that man pages stay in sync with their sources.

Therefore, someone (David) has to manually update the man pages every month by running bin/rake man:build.

With this, having actions doing this can help save manual work.

Closes: #3582

Signed-off-by: Utkarsh Gupta <utkarsh@debian.org>


Tasks:

  • Describe the problem / feature
  • Write code to solve the problem

I will abide by the code of conduct.

At the beginning of every month, the CI breaks
as it has a check that makes sure that man pages
stay in sync with their sources.
Therefore, someone (David) has to manually update
the man pages every month by running
`bin/rake man:build`.

With this, having an actions doing this, can help
save manual work.

Closes: rubygems#3582

Signed-off-by: Utkarsh Gupta <utkarsh@debian.org>
@bundlerbot bundlerbot added the CI CI related issues label Jun 21, 2020
@utkarsh2102
Copy link
Contributor Author

Potential solutions:

  • Exclude "month header" regeneration from the bin/rake man:build task.
  • Exclude "month header" checking from the bin/rake man:check task.
  • Automatically commit the result of bin/rake man:build to master once a month.

My preferred solution would be 3, but it sounds like tricky github actions stuff.

Hi @deivid-rodriguez,
I took a stab at this and did it in the way as you preferred 😄

I am not sure, but I hope this helps 😅

Copy link
Member

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

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

This is so awesome, I'm really excited about this!! 🎉

.github/workflows/monthly.yml Outdated Show resolved Hide resolved
.github/workflows/monthly.yml Outdated Show resolved Hide resolved
.github/workflows/monthly.yml Show resolved Hide resolved
.github/workflows/monthly.yml Outdated Show resolved Hide resolved
.github/workflows/monthly.yml Outdated Show resolved Hide resolved
.github/workflows/monthly.yml Outdated Show resolved Hide resolved
.github/workflows/monthly.yml Outdated Show resolved Hide resolved
This commit fixes the workflow in various ways:
- Don't rely on external action.
- Set `working_directory:` wherever needed.
- Install `ronn` instead of running `rake setup`.
- Use `ruby-version:` as 2.7.1.
- Set `timeout-minutes:` to 15.
- Drop `push` part, only scheduling part is needed.
- Change author to "The Bundler Bot".

Signed-off-by: Utkarsh Gupta <utkarsh@debian.org>
Copy link
Member

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

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

This looks good, but we could try it first. You can temporarily enable Github Actions in your fork, and then add

`on:
  push:
    branches:
      - man-page-issue 

Then once you push, the workflow should trigger. Everything should run just fine, except for the git push which I'm uncertain whether it will succeed or not.

Could you try it?

@utkarsh2102
Copy link
Contributor Author

Of course. Shall do! :)

@utkarsh2102
Copy link
Contributor Author

Copy link
Member

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

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

Indeed, thanks! Let me just double check that we can push to master directly in this repo, and that other maintainers are ok with it, and I'll merge it! Thanks so much!!

@indirect
Copy link
Member

@deivid-rodriguez sorry, I think your wrong permissions were my fault. Fixed! There is nothing blocking pushes to master, just a requirement that tests pass to allow merges.

@deivid-rodriguez
Copy link
Member

No problem, thanks! I have proper permissions now :)

So, you're ok with this @indirect?

By the way, I noticed that the "require status checks to pass" is indeed checked, but no specific checks are selected, so that's essentially the same as not being checked 😅. I'll fix it!

@indirect
Copy link
Member

@deivid-rodriguez yes, this seems good! I thought "require checks to pass" would allow if tests never ran, but block if a test failed? But maybe not. 😅

@deivid-rodriguez
Copy link
Member

I believe the way it works is it prevent merges unless all of the status checks checked in the list available below pass. But since we have none checked, I guess it always lets you merge. From what I recall from other repos, when merges are prevented, either the Merge button is disabled, or displayed in red (if "allow administrators to override these requirements" is checked).

I think this behaviour is consistent with what I see in #3758, for example.

Copy link
Member

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

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

Thanks @utkarsh2102! Let's try this!

@deivid-rodriguez deivid-rodriguez merged commit da602ea into rubygems:master Jun 25, 2020
@utkarsh2102 utkarsh2102 deleted the man-page-issue branch June 25, 2020 10:00
@deivid-rodriguez
Copy link
Member

@indirect I just merged a PR with failing status checks: #3762, just to verify this (and because the CI failure was clearly unrelated, and on an entry that shouldn't actually be required). It indeed merged without issues. May I go ahead and check the proper boxes in our configuration to prevent merging PRs with failed required checks?

@indirect
Copy link
Member

@deivid-rodriguez yes! go for it. we probably should have checked those boxes when we stopped using Bors 😬

@deivid-rodriguez
Copy link
Member

Indeed! Better late than never though :)

@deivid-rodriguez
Copy link
Member

Done! 👍

@deivid-rodriguez
Copy link
Member

Funny enough, enabling these checks yesterday broke the first run of this automated task today: https://github.com/rubygems/rubygems/runs/824703487?check_suite_focus=true.

2020-07-01T00:36:34.0538173Z remote: error: GH006: Protected branch update failed for refs/heads/master.        
2020-07-01T00:36:34.0538683Z remote: error: 56 of 56 required status checks are expected.        
2020-07-01T00:36:34.0553069Z To https://github.com/rubygems/rubygems
2020-07-01T00:36:34.0553786Z  ! [remote rejected] master -> master (protected branch hook declined)
2020-07-01T00:36:34.0554113Z error: failed to push some refs to 'https://github.com/rubygems/rubygems'

I believe you can whitelist certain users to be able to commit directly to protected branches, so it probably makes sense to use that feature with bundlerbot.

@deivid-rodriguez
Copy link
Member

Actually, from reading what the option does, I think it's not going to work:

  • Restrict who can push to matching branches.

Specify people, teams or apps allowed to push to matching branches. Required status checks will still prevent these people, teams and apps from merging if the checks fail.

So we'll want to either disable these checks again or do something different like creating a PR instead of pushing directly to master.

@deivid-rodriguez deivid-rodriguez mentioned this pull request Jul 2, 2020
4 tasks
@utkarsh2102
Copy link
Contributor Author

So we'll want to either disable these checks again or do something different like creating a PR instead of pushing directly to master.

Aw, too bad :/
But we could do something like:

  • Actions to create a PR
  • Actions to merge that (maybe after the checks have passed or something of that sort!)

Of course, this will be a single actions, but probably we'll have to rely on some external actions.
D'you think that would suffice?

@utkarsh2102
Copy link
Contributor Author

Hahaha, only today I was rebasing my branches and I found this: utkarsh2102@fda7273

The Bundler Bot at least pushed to my master for bumping the man-page month and it worked alright :P
(nice to know!) 😄

@deivid-rodriguez
Copy link
Member

Hi @utkarsh2102!

That would be awesome indeed. As a matter of fact, having some sort of "Merge after CI passes" through an action would be useful in general, maybe by tagging the PR with a label, or commenting something specific. We used to have something similar back when we used bors-ng, so that commenting @bundlerbot merge would run CI and merge the PR. However, we stopped using it because it had many issues.

@utkarsh2102
Copy link
Contributor Author

Aha, I see!
Okay, I am going to take a stab at this in the coming days and hopefully come up with something! 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI CI related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Man pages check break CI every month
5 participants