mozilla / FirefoxColor

Theming demo for Firefox Quantum and beyond
https://color.firefox.com
Mozilla Public License 2.0
452 stars 93 forks source link

Set protected status on production branch #368

Closed moz-hwine closed 5 years ago

moz-hwine commented 6 years ago

The production branch on this repository is not protected against force pushes. This setting is recommended as part of Mozilla's Guidelines for a Sensitive Repository.

Anyone with admin permissions for this repository can correct the setting using this URL.

If you have any questions, or believe this issue was opened in error, please contact us and mention SOGH0001 and this repository.

Thank you for your prompt attention to this issue. --Firefox Security Operations team

lmorchard commented 6 years ago

We use force push on production, stage, and development branches to set the exact commit we want to deploy.

This allows us to easily cherry pick hotfixes from the master branch when it might contain other changes we don't want to deploy yet, then later update the production branch to match master when we are ready.

hwine commented 6 years ago

Another approach that achieves the same goal is to use tags for those deploy points, if I understand correctly.

Any concerns with using tags, so the branches can be protected?

lmorchard commented 6 years ago

We haven't started using tags on this project yet, but on others we force-push the tag commits. I'm not sure how tags get around the protection. (i.e. We might tag a branch other than master with cherry-picked commits, then force-push that tag to production)

hwine commented 6 years ago

Ah - the pattern would be:

The other advantage is it keeps a clear history of everything that was deployed. IIUC, you may overwrite a previous deployment point with the force push.

lmorchard commented 6 years ago

tl;dr: I think I'm still just confused That kind of process has historically led us into merge conflicts very quickly and sometimes introduces errors when manual resolution happens. That's why we moved to force-pushing the exact tested commits from other branches & tags. We treat production branch as basically a utility and not as a history-keeping mechanism

hwine commented 6 years ago

That's what the (not well know) "ours" merge strategy is for. You can have your cake and eat it too! (It also means folks get the right stuff with just fast forward updates.)

This answer has some examples of using merge strategies in cherry pick situation that sound similar to yours.

lmorchard commented 6 years ago

deploy from the tag

This is what I think I'm confused by. Our infrastructure doesn't deploy from tags. It watches the production branch and deploys when HEAD changes - not entirely unlike GitHub Pages and the gh-pages branch.

Regardless of merging strategies - it sounds like the real issue is that you need us to change to a system that accepts the submission of a tag, rather than watching a branch for changes.

Any concerns with using tags, so the branches can be protected?

So I think the concern is that our current system doesn't support deployment from tags.

hwine commented 6 years ago

Regardless of merging strategies - it sounds like the real issue is that you need us to change to a system that accepts the submission of a tag, rather than watching a branch for changes.

That's not what we're asking for today. Today is just about branch protection. We understand there may be future details to be worked out.

lmorchard commented 6 years ago

If the branch is protected, we can't force-push the commit from a tag to exactly match that tag. And also, I guess the implied thing there is we'd prefer not to wrestle with merge conflicts when deploying, no matter the strategy.

lmorchard commented 6 years ago

we'd prefer not to wrestle with merge conflicts when deploying, no matter the strategy.

Just to elaborate on this, because I could be wrong / overthinking it. But basically: When I have a commit (or a tag), I can point to the test run in CI where things passed.

If I force-push that exact commit to production, I know it's exactly the same as what passed.

If I instead try to merge into production branch, then I have a new commit. If everything went well with the merge, then that commit ends up in the same condition as if I'd force-pushed (just with a different history). If things didn't go well, then we have a new & untested commit with errors on its way to production. (Actually, that should be an bug if true - tests should run on production deploy before the results actually get deployed, and so errors should block deployment.)

If I play with the "ours" strategy, maybe I'll get confident that the results are the same. So far, we've just force-pushed to keep things simple

clouserw commented 6 years ago

This is the same (documented: https://github.com/mozilla/testpilot/blob/master/docs/development/deployment.md#deploy-production ) way we deploy most Test Pilot projects. Hal and I have chatted about it a bit and I'm going to close this for now, with the understanding that we'll probably have the discussion again in the future and maybe can come up with a better way to do it. Note that we'll need to get ops involved also since they handle those deploy scripts. Thanks!

moz-hwine commented 5 years ago

Hello! This is your neighborhood secops team still looking out for you!

The production branch on this repository is still not protected against force pushes. If the repository's production branches are not set as the GitHub default, please fill out this form.

If you have any questions, or believe this issue was opened in error, please contact us and mention SOGH001-1 and this repository.

Thank you for your prompt attention to this issue. --Firefox Operations Security team

moz-hwine commented 5 years ago

Hello! This is your neighborhood secops team still looking out for you!

The production branch on this repository is still not protected against force pushes. If the repository's production branches are not set as the GitHub default, please fill out this form.

If you have any questions, or believe this issue was opened in error, please contact us and mention SOGH001-2 and this repository.

Thank you for your prompt attention to this issue. --Firefox Operations Security team

moz-hwine commented 5 years ago

branch now protected