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 an operators group #220

Merged
merged 1 commit into from Feb 12, 2016
Merged

Add an operators group #220

merged 1 commit into from Feb 12, 2016

Conversation

@larsbergstrom
Copy link
Contributor

larsbergstrom commented Feb 11, 2016

r? @Manishearth

Manish, do you have any idea how I can test this without dropping things on their head? The goal here is to have a "role" for operators, separate from that of reviewers (even if it's the same thing under the hood) to let people self-retry without waiting for delegate permissions.

Review on Reviewable

@KiChjang
Copy link
Member

KiChjang commented Feb 11, 2016

Doesn't the try group already allow members to retry?

@larsbergstrom
Copy link
Contributor Author

larsbergstrom commented Feb 11, 2016

Aha, I'm wrong - it's for delegated r+, not retry. Thanks, @KiChjang!

@Manishearth
Copy link
Member

Manishearth commented Feb 11, 2016

Try allows for retry, but not for re-r+ given a change.

We don't need to test this, we can deploy it and check that the generated cfg.toml is as expected.

@aneeshusa
Copy link
Member

aneeshusa commented Feb 11, 2016

"We don't need to test this" makes me worried 😟

I agree that checking the generated cfg.toml is a sufficient test for this; I'd suggest doing that in a Vagrant VM before deploying to production. You can also run a highstate with test=True to see the expected changes (although in this case it looks like the generated file will be the same afterwards).

@Manishearth
Copy link
Member

Manishearth commented Feb 11, 2016

Oh, we always do test=True before deploying 😄

I meant that we don't need to test this by opening a dummy PR and having Bobby review it. We'll do the regular salt tests of course.

@larsbergstrom
Copy link
Contributor Author

larsbergstrom commented Feb 12, 2016

I confirmed with the nifty Vagrant wizardry that this works and generates a good cfg.toml for homu.

r? @Manishearth

@Manishearth
Copy link
Member

Manishearth commented Feb 12, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Feb 12, 2016

📌 Commit 09a3b11 has been approved by Manishearth

@bors-servo
Copy link
Contributor

bors-servo commented Feb 12, 2016

Testing commit 09a3b11 with merge 0697939...

bors-servo added a commit that referenced this pull request Feb 12, 2016
Add an operators group

r?  @Manishearth

Manish, do you have any idea how I can test this without dropping things on their head? The goal here is to have a "role" for operators, separate from that of reviewers (even if it's the same thing under the hood) to let people self-retry without waiting for delegate permissions.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/saltfs/220)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Feb 12, 2016

☀️ Test successful - travis

@bors-servo bors-servo merged commit 09a3b11 into servo:master Feb 12, 2016
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.