Skip to content

Commit

Permalink
Allow members to change project visibility
Browse files Browse the repository at this point in the history
  • Loading branch information
@eclipse-opendut-bot
eclipse-opendut-bot committed Dec 20, 2023
1 parent 358a9b9 commit bb5047e
Showing 1 changed file with 2 additions and 4 deletions.
6 changes: 2 additions & 4 deletions otterdog/eclipse-opendut.jsonnet
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ orgs.newOrg('eclipse-opendut') {
settings+: {
dependabot_security_updates_enabled_for_new_repositories: false,
description: "Test Electronic Control Units around the world in a transparent network.",
members_can_change_project_visibility: false,
name: "Eclipse openDuT",
packages_containers_internal: false,
packages_containers_public: false,
Expand Down Expand Up @@ -53,9 +52,8 @@ orgs.newOrg('eclipse-opendut') {
],
requires_pull_request: false,
required_approving_review_count: null,
required_status_checks+: [
"build"
],
requires_status_checks: false,
required_status_checks: [],
requires_commit_signatures: false,
}
],
Expand Down

11 comments on commit bb5047e

@mbfm
Copy link
Contributor

@mbfm mbfm commented on bb5047e Jan 19, 2024

Choose a reason for hiding this comment

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

Hey @netomi, the bot undid the status check that you built for us.

As a reminder, you had added the status check in the commit before: 358a9b9
...which we had discussed beforehand here: #4 (comment)

Is this something that needs to be fixed in the bot?

@netomi
Copy link
Contributor

@netomi netomi commented on bb5047e Jan 19, 2024

Choose a reason for hiding this comment

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

That was a temporary fix to get things to a working state the status check build was removed as it was referencing the wrong check, see the latest commit which is correct imho:

c51dd54

@mbfm
Copy link
Contributor

@mbfm mbfm commented on bb5047e Jan 19, 2024

Choose a reason for hiding this comment

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

The commit you referenced changes the status check name for the branch protection rule of the main branch.
But the bot removed it from the repository ruleset for protecting the development branch.

@netomi
Copy link
Contributor

@netomi netomi commented on bb5047e Jan 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I had to read up on that, this change was due to this conversation when creating the ruleset: #5 (comment)

So after that I pushed the change to the repo with our command line tool which impersonates as the bot user that why you see this commit.

@mbfm
Copy link
Contributor

@mbfm mbfm commented on bb5047e Jan 19, 2024

Choose a reason for hiding this comment

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

But in that conversation, I agreed with you that having the build status check (or nowadays status-check) would be a good idea.

And it wasn't you, who removed the line. It was done by @eclipse-opendut-bot via some automatic rule in this commit that I'm commenting on.
I'm not sure, if you're reading this from some alternative UI, so here is specifically the commit where the bot removed the given line: bb5047e#diff-c354ac7b09291463d238ddf321bf7549c1c68d379c4aa6cbfd9d448d03f74d9dL57

If you don't think the bot would do that automatically, we can just add it back and see what happens.
We did have a pull request for development earlier, which did not require the status check to complete, so that is at least not behaving as we'd ideally like to have it: eclipse-opendut/opendut#37

@netomi
Copy link
Contributor

@netomi netomi commented on bb5047e Jan 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Afaicr, your workflow is that people commit to the development branch either directly or via pull requests. If you specify status checks that are required to be able to update the branch, you cannot use branch protection rules as they are designed that status checks only work with pull requests (see also https://otterdog.readthedocs.io/en/latest/reference/organization/repository/ruleset/). Now the idea was to use a ruleset instead which allows to model that, however to enable status checks with a ruleset you explicitly have to specify also people that can bypass the status checks as they will not be attached when directly pushing to the branch. For reasons of simplicity and from the discussion referenced above, I removed the check again to unblock you.

However, if I misunderstood you and you would like to have that status check also enabled for the development branch you can re-enable it, but you will also have to specify all actors with Write access for the bypass list as otherwise a simple push will not work anymore. Here is an example for a ruleset like that for another project: https://github.com/eclipse-zenoh/.eclipsefdn/blob/main/otterdog/eclipse-zenoh.jsonnet#L3

For the commit: I did it with the cli of otterdog that uses the token of the bot to make commits, so I am 100% sure I did this myself as the bot does not update the repo automatically.

@netomi
Copy link
Contributor

@netomi netomi commented on bb5047e Jan 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

However, if you specify a ruleset with a required status check and let all users with write permission bypass, the required status is also useless as everybody can bypass it.
The PR will still show it as required, but anybody can still merge the PR (though you have to select bypass and the PR will show it). So its up to you, but afaict you cant really model a protection rule with required status check and let every user still push directly.

@mbfm
Copy link
Contributor

@mbfm mbfm commented on bb5047e Jan 22, 2024

Choose a reason for hiding this comment

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

Thanks for the elaborate response.

I'm not sure, if we had a misunderstanding in December, or if this something new you've learned in the meantime, but in this conversation: #5 (comment)
...you said that:

If you directly push to the development branch, [the status check] would not be taken into account.

If that statement is still correct, then it seems like this is what we want and that we actually wouldn't need this Write access bypass.

We still don't have many people actually directly pushing to development, so unless you're sure that this can't work, I would have no problem just trying it out. I can then open up a PR to change it as I imagine, it was supposed to work at the end of last year...

@netomi
Copy link
Contributor

@netomi netomi commented on bb5047e Jan 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the elaborate response.

I'm not sure, if we had a misunderstanding in December, or if this something new you've learned in the meantime, but in this conversation: #5 (comment) ...you said that:

If you directly push to the development branch, [the status check] would not be taken into account.

If that statement is still correct, then it seems like this is what we want and that we actually wouldn't need this Write access bypass.

The statement is not fully correct and I mixed up things when I said it. The status check will always be required no matter if you directly push or create a PR. Now with branch protection rules, you cant even bypass it, so thats where a ruleset come into play.
With the bypass actors you can bypass a status check and this is what we set up for some projects:

https://github.com/eclipse-zenoh/.eclipsefdn/blob/main/otterdog/eclipse-zenoh.jsonnet#L3
https://github.com/jetty/.eclipsefdn/blob/main/otterdog/jetty.jsonnet#L381

For the jetty project for example it makes perfect sense, as the list of bypassers is only a subset of all committers. In your case as in zenoh all committers would be able to bypass.
A rule like that would basically tell you that a certain check is required, but if you really have to you can still bypass and all committers can directly push to the development branch as it makes sense atm.
The rule prevents force pushes though and this is what we ultimately wanna enforce.

We still don't have many people actually directly pushing to development, so unless you're sure that this can't work, I would have no problem just trying it out. I can then open up a PR to change it as I imagine, it was supposed to work at the end of last year...

I am perfectly fine to adjust as you want if it fits your use-case. As I said our main concern is to prevent force pushes and ideally advocate reviews buts that depends on the state of the project ofc.

@mbfm
Copy link
Contributor

@mbfm mbfm commented on bb5047e Jan 22, 2024

Choose a reason for hiding this comment

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

Alright, I discussed this with my colleagues, whether this is actually what we want/need, and it seems like it's not.

We had gotten a PR from someone who's not a maintainer and the GitHub UI didn't show the checks inline.
But it seems like that was rather the case, because GitHub doesn't want to just execute the code from strangers on our GitHub runners, which makes sense.

So, we'll continue working with the configuration as it is, for now.
Thanks for your help anyways. 🙂

@netomi
Copy link
Contributor

@netomi netomi commented on bb5047e Jan 22, 2024

Choose a reason for hiding this comment

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

so if you receive a PR from an outside contributor workflows are not automatically run but rather have to be manually approved, see https://docs.github.com/en/actions/managing-workflow-runs/approving-workflow-runs-from-public-forks

There are settings to control that on a repo level, e.g. if a contributor always needs manual approval or just for the first PR:

image

Please sign in to comment.