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

uninstall: always yes #78

Merged
merged 1 commit into from
Oct 12, 2022
Merged

uninstall: always yes #78

merged 1 commit into from
Oct 12, 2022

Conversation

liyishuai
Copy link
Member

in case after_script installed some dependants

in case after_script installed some dependants
@liyishuai liyishuai requested a review from erikmd October 12, 2022 05:32
@liyishuai liyishuai merged commit 44a09e0 into master Oct 12, 2022
@liyishuai liyishuai deleted the liyishuai-patch-1 branch October 12, 2022 08:38
@erikmd
Copy link
Member

erikmd commented Oct 13, 2022

Thanks @liyishuai, LGTM… but the PR is incomplete → you should also update:

opam remove coq-proj

opam remove $PACKAGE

the string `opam remove $PACKAGE`).

Then, I'll do a release.

@erikmd
Copy link
Member

erikmd commented Oct 13, 2022

And even if some changes are very small like these, I prefer that the PRs are merged by the maintainers (@Zimmi48 @erikmd), as it is the standard practice in coq-community (?)

@erikmd erikmd added the enhancement New feature or request label Oct 13, 2022
@Zimmi48
Copy link
Member

Zimmi48 commented Oct 13, 2022

And even if some changes are very small like these, I prefer that the PRs are merged by the maintainers (@Zimmi48 @erikmd), as it is the standard practice in coq-community (?)

Some repositories are more lax than others. We could set up branch protection rules here. WDYT?

@erikmd
Copy link
Member

erikmd commented Oct 13, 2022

I thought every master branch of every coq-community project was protected like this, so it appears this is not the case;
so yes, I will configure this now. [Edit: done]

@Zimmi48
Copy link
Member

Zimmi48 commented Oct 13, 2022

Maybe we should document somewhere (manifesto FAQ?) what the default is and what the options are? cc @palmskog

@palmskog
Copy link
Member

My view: branch protection is definitely something that maintainers can decide about, and we should probably set it up for all "critical" projects, such as this and the other Docker projects (and maybe use it for all Coq Platform projects as well?).

But good to document this, and also best practices regarding merging PRs.

@erikmd
Copy link
Member

erikmd commented Oct 13, 2022

@palmskog Yes;

FTR, here is the screenshot of the settings currently applied for coq-community/docker-coq-action's master branch:

2022-10-13_github_coq-community_docker-coq-action_protected_branches

@palmskog
Copy link
Member

palmskog commented Oct 14, 2022

@erikmd do you really have to add people explicitly under "People, teams or apps with push access"? I thought we could have the convention that everyone that is supposed to have push access is either an Admin or a Coq-community member with the Maintain role [for the repo in question].

@Zimmi48
Copy link
Member

Zimmi48 commented Oct 14, 2022

It's actually useless to repeat people here since the list already includes all organization or repository admins.

It is also not useful to check "Restrict pushes that create matching branches" for a pattern which is the branch name of an already existing branch. This is more useful when you want to prevent any other branch, or any new release branch from being created on the repository, like we do in Coq. Here this would be useful for instance if you transformed the v1 rule into v[0-9].* or something like this (I don't recall the exact syntax).

Anyway, this highlights even more the need to document this.

@erikmd
Copy link
Member

erikmd commented Oct 14, 2022

@palmskog @Zimmi48 thanks for your comments, yes I was already aware that the crossed checkboxes of the config I shared were sub-optimal, but it didn't induce any bug… admittedly, if we want to document a standard, cross-organization configuration, we should better have a polished one.
Anyway while we are on it, I refactored the config of docker-coq-action accordingly, and replaced v1 with v* (doc1),(doc2)

Regarding the checkbox Require signed commits, it looked important to me for docker-coq-action, but I wouldn't put it forth for other coq-community projects at first sight.

@erikmd erikmd assigned erikmd and unassigned erikmd Oct 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants