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

Update contributing guide #1196

Merged
merged 1 commit into from
Oct 30, 2023
Merged

Update contributing guide #1196

merged 1 commit into from
Oct 30, 2023

Conversation

hynek
Copy link
Member

@hynek hynek commented Oct 30, 2023

Here's a PR @webknjaz – I even made it out of sync with main. ;)

Copy link
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

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

🎉

@webknjaz webknjaz enabled auto-merge October 30, 2023 10:09
@webknjaz
Copy link
Member

@hynek drop the requirement for the branches to be rebased.

@hynek
Copy link
Member Author

hynek commented Oct 30, 2023

@hynek drop the requirement for the branches to be rebased.

what exactly does that mean? I've switched off the need update 🤔:
CleanShot 2023-10-30 at 11 14 00@2x

Is it linear history?

@hynek
Copy link
Member Author

hynek commented Oct 30, 2023

linear history off doesn't work either

@webknjaz webknjaz disabled auto-merge October 30, 2023 14:31
@webknjaz
Copy link
Member

what exactly does that mean? I've switched off the need update 🤔: CleanShot 2023-10-30 at 11 14 00@2x

Should be this one. Could you make a full page screenshot? Do you have multiple branch protection rules? What do they look like?

@webknjaz
Copy link
Member

This is weird:

Screenshot_2023-10-30-15-35-28-16_40deb401b9ffe8e1df2f1cc5ba480b12

Did you accidentally select the "Lock" checkbox?

@hynek
Copy link
Member Author

hynek commented Oct 30, 2023

This is no productive workflow, I've given you admin privileges please look yourself. ;)

@hynek
Copy link
Member Author

hynek commented Oct 30, 2023

but yeah lock branch was always on, so nobody pushes directly.

@webknjaz
Copy link
Member

This is no productive workflow, I've given you admin privileges please look yourself. ;)

Okay, will do :)

@webknjaz
Copy link
Member

but yeah lock branch was always on, so nobody pushes directly.

That doesn't sound right. It's supposed to block virtually any updates, unless you merge bypassing the restrictions..

@webknjaz
Copy link
Member

@hynek so yeah, I think Lock branch is at fault. What you want should already be covered by Require a pull request before merging, except you added yourself Allow specified actors to bypass required pull requests which may cause a confusing impression that it might not work.

I've unchecked Lock branch and everything should work now.
Moreover, I inspected the audit log and found out that you enabled this checkbox 3 days ago. See for yourself: https://github.com/organizations/python-attrs/settings/audit-log?q=+action%3Aprotected_branch.update_lock_branch_enforcement_level — you added the lock, I removed it and there's no other changes related to this toggle in the log. If it was in place for longer, nobody would be able to update main (i.e. merge PRs) and it'd be noticed much sooner.

@webknjaz
Copy link
Member

@hynek I'll leave clicking "Merge when ready" to you so you could experience how this works.

@hynek hynek added this pull request to the merge queue Oct 30, 2023
@hynek
Copy link
Member Author

hynek commented Oct 30, 2023

stares excitedly

Merged via the queue into main with commit 52c1cd9 Oct 30, 2023
18 checks passed
@hynek hynek deleted the add-warning branch October 30, 2023 15:00
@hynek
Copy link
Member Author

hynek commented Oct 30, 2023

It didn't trigger the pypi workflow but I think that's OK. I've removed the requirement.

@webknjaz
Copy link
Member

@hynek ah, this https://github.com/python-attrs/attrs/actions/runs/6694666592? It still happened post-merge, on push. I suppose you may want to update the event triggers there to do what you want. Such disconnect between workflows is one of the reasons I've started migrating towards a “main” workflow with all the events that ties together reusable “modules” that don't have other event triggers except for workflow_call.

@hynek
Copy link
Member Author

hynek commented Oct 30, 2023

I've been doing something different lately which is similar to your sdist workflow but goes a step further: https://github.com/hynek/svcs/blob/84726ff72eb080f6464e1ec8f9d25912eb3230a8/.github/workflows/ci.yml#L21-L66

I'll move attrs to it when… I have time 🤡

@webknjaz
Copy link
Member

@hynek it's actually pretty much the same AFAICS

@webknjaz
Copy link
Member

@hynek did you replace the action with a tar invocation because you also wanted a wheel? I like to download both wheel and sdist. Use sdist instead of a Git checkout and pass the wheel to --installpkg. Did you not like having to download the GHA artifact twice? If yes, I think it might be reasonable to factor that into my action's API and maybe expose the wheel from the same artifact additionally. WDYT?

@hynek
Copy link
Member Author

hynek commented Oct 31, 2023

Yeah the wheel was the reason. Given I want to use my own builder/linter it didn't give me much if anything. I don't remember the exact details anymore, but I'm not sure what I'd need an opaque action for? Like what could I save compared to status quo? it's just a download + un-tar. 🤔

But yes, "pre-built wheels for code, everything else from sdist" is the way to go IMHO. Nothing gets closer to what we ship so I really like it.

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

Successfully merging this pull request may close these issues.

2 participants