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

skip_report flag in prowjob only used by crier reporters that report back to CI system #22486

Merged
merged 1 commit into from
Jun 9, 2021

Conversation

chaodaiG
Copy link
Contributor

@chaodaiG chaodaiG commented Jun 8, 2021

skip_report initially was meant for skipping reporting on github as comment at

// SkipReport skips commenting and setting status on GitHub.
. This has later on been used for skipping all crier reporters as
if !pj.Spec.Report || !r.reporter.ShouldReport(ctx, log, &pj) {
and
pjs.Report = !p.SkipReport
.

gcsreporter is responsible for uploading finished.json when sidecar failed to do so, as well as uploading prowjob.json. These should happen independent of skip_report.

This PR classifies crier reporters to be either report back to or not to CI system, and skip_report is only respected by reporters that report back to CI system.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. area/prow Issues or PRs related to prow area/prow/crier Issues or PRs related to prow's crier component sig/testing Categorizes an issue or PR as relevant to SIG Testing. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 8, 2021
@k8s-ci-robot k8s-ci-robot requested review from cjwagner and fejta June 8, 2021 22:16
@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jun 8, 2021
@chaodaiG
Copy link
Contributor Author

chaodaiG commented Jun 8, 2021

@fejta
Copy link
Contributor

fejta commented Jun 8, 2021

This PR is unnecessary

@chaodaiG
Copy link
Contributor Author

chaodaiG commented Jun 8, 2021

This PR is unnecessary

Had synced offline with @fejta, and updated PR description with more context, also this PR is open to discussion

@chaodaiG
Copy link
Contributor Author

chaodaiG commented Jun 8, 2021

The other reporter could benefit from this is pubsub reporter

@chaodaiG chaodaiG force-pushed the crier-always-upload branch from 7bdf044 to 9a8c75d Compare June 8, 2021 23:09
@chaodaiG chaodaiG changed the title Crier supports only skip reporting github skip_report flag in prowjob only used by crier reporters that report back to CI system Jun 8, 2021
@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 8, 2021
Copy link
Contributor

@fejta fejta left a comment

Choose a reason for hiding this comment

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

/hold

If we're concerned about safety we could add a reporter.AlwaysTryReport() or something, but maybe not worth it

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 8, 2021
@chaodaiG chaodaiG force-pushed the crier-always-upload branch from 9a8c75d to 9e1e046 Compare June 8, 2021 23:16
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 8, 2021
@chaodaiG
Copy link
Contributor Author

chaodaiG commented Jun 8, 2021

the previous commit marked gcs/kubernetes reporter instead of gerrit reporter as CI reporter, fixed by force push

@chaodaiG chaodaiG force-pushed the crier-always-upload branch from 9e1e046 to 1bc0635 Compare June 8, 2021 23:32
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jun 8, 2021
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 8, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chaodaiG, fejta

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@chaodaiG
Copy link
Contributor Author

chaodaiG commented Jun 9, 2021

@alvaroaleman , any objection before I unhold this PR?

@alvaroaleman
Copy link
Member

no, sounds good to me. It makes the meaning of this field a bit confusing (maybe godoc that reporters themselves decide if they respect it?) but having something like k8s reporter always report makes a lot of sense.
/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 9, 2021
@k8s-ci-robot k8s-ci-robot merged commit 2eb997a into kubernetes:master Jun 9, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.22 milestone Jun 9, 2021
@chaodaiG chaodaiG deleted the crier-always-upload branch June 10, 2021 18:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/prow/crier Issues or PRs related to prow's crier component area/prow Issues or PRs related to prow cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants