Skip to content
This repository has been archived by the owner on Mar 22, 2021. It is now read-only.

spec: also set required context on PRs #30

Closed
wants to merge 1 commit into from
Closed

Conversation

jlebon
Copy link
Collaborator

@jlebon jlebon commented Mar 10, 2017

For the same reasons the 'required' context is useful for branches, it
can also be useful for PRs: it allows repo admins to set a single status
as required in the branch protection settings rather than multiple,
possibly changing, statuses.

This also specifically helps with status-based exemptions in Homu: by
having the required context set on the PR, Homu will be able to
determine whether it can safely skip re-testing the code on the auto
branch.

@jlebon
Copy link
Collaborator Author

jlebon commented Mar 10, 2017

@cgwalters, WDYT? I could make it another setting I suppose so that repos that don't use homu don't have another status. (I don't mind them, though I know you had some reservations).

This also paves the way towards only having the required context on PRs. Since it links to a summary page, users would still be able to access all the results (which was discussed in #18). My concerns in #18 are still present, but we could at least make it an option for repos where it makes sense.

@jlebon jlebon force-pushed the pr/required-on-prs branch from dc92027 to 7ce8999 Compare July 7, 2017 16:06
@jlebon
Copy link
Collaborator Author

jlebon commented Jul 7, 2017

Rebased. I've been thinking about this today. I think this is worth it just for the savings in test resources alone, esp. given that we have a lot of hungry testsuites nowadays. Though being able to use it in branch protection settings is really nice too.

@cgwalters
Copy link
Member

Those repos would just get the new status if they used required right? I guess if we really cared we could add a papr config for whether homu was in use...or of course merge them on the infra side more as we talked about.

Anyways seems sane to me!

For the same reasons the 'required' context is useful for branches, it
can also be useful for PRs: it allows repo admins to set a single status
as required in the branch protection settings rather than multiple,
possibly changing, statuses.

This also specifically helps with status-based exemptions in Homu: by
having the required context set on the PR, Homu will be able to
determine whether it can safely skip re-testing the code on the auto
branch.
@jlebon
Copy link
Collaborator Author

jlebon commented Sep 27, 2017

Rebased! ⬆️

Those repos would just get the new status if they used required right?

Yes, that's correct!

or of course merge them on the infra side more as we talked about.

Yeah, part of #62 is definitely to make these kinds of integrations smoother. Though it's easy enough to support in the meantime.

@cgwalters
Copy link
Member

LGTM

@jlebon
Copy link
Collaborator Author

jlebon commented Sep 27, 2017

@rh-atomic-bot r=cgwalters 78b573b

1 similar comment
@jlebon
Copy link
Collaborator Author

jlebon commented Sep 27, 2017

@rh-atomic-bot r=cgwalters 78b573b

@rh-atomic-bot
Copy link

🙀 78b573b is not a valid commit SHA. Please try again with 7ce8999.

@jlebon jlebon closed this Sep 27, 2017
@jlebon jlebon reopened this Sep 27, 2017
@rh-atomic-bot
Copy link

⌛ Testing commit 78b573b with merge 9d5f60b...

@rh-atomic-bot
Copy link

☀️ Test successful - status-papr
Approved by: cgwalters
Pushing 9d5f60b to master...

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

Successfully merging this pull request may close these issues.

3 participants