Remove dead checks in image promoter #796
Conversation
k8s-ci-robot
added
release-note-none
|
Hi @jonjohnsonjr. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
k8s-ci-robot
added
sig/release
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
| // Run executes ImageRemovalCheck on a set of promotion edges. | ||
| // Returns an error if the pull request removes images from the | ||
| // promoter manifests. | ||
| func (check *ImageRemovalCheck) Run() error { | ||
| r, err := gogit.PlainOpen(check.GitRepoPath) | ||
| if err != nil { | ||
| return fmt.Errorf("could not open the Git repo: %v", err) | ||
| } | ||
| w, err := r.Worktree() | ||
| if err != nil { | ||
| return fmt.Errorf("could not create Git worktree: %v", err) | ||
| } | ||
|
|
||
| // The Prow job that this check is running in has already cloned the | ||
| // git repo for us so we can just checkout the master branch to get the | ||
| // master branch's version of the promoter manifests. | ||
| err = w.Checkout(&gogit.CheckoutOptions{ | ||
| Hash: check.MasterSHA, | ||
| Force: true, | ||
| }) | ||
| if err != nil { | ||
| return fmt.Errorf("could not checkout the master branch of the Git"+ | ||
| " repo: %v", err) | ||
| } | ||
|
|
||
| mfests, err := schema.ParseThinManifestsFromDir(check.GitRepoPath, false) | ||
| if err != nil { | ||
| return fmt.Errorf("could not parse manifests from the directory: %v", | ||
| err) | ||
| } | ||
| masterEdges, err := ToPromotionEdges(mfests) | ||
| if err != nil { | ||
| return fmt.Errorf("could not generate promotion edges from promoter"+ | ||
| " manifests: %v", err) | ||
| } | ||
|
|
||
| // Reset the current directory back to the pull request branch so that this | ||
| // check doesn't leave lasting effects that could affect subsequent checks. | ||
| err = w.Checkout(&gogit.CheckoutOptions{ | ||
| Hash: check.PullRequestSHA, | ||
| Force: true, | ||
| }) | ||
| if err != nil { | ||
| return fmt.Errorf("could not checkout the pull request branch of the"+ | ||
| " Git repo %v: %v", | ||
| check.GitRepoPath, err) | ||
| } | ||
|
|
||
| return check.Compare(masterEdges, check.PullEdges) | ||
| } |
There was a problem hiding this comment.
This check actually seems useful to have if it works. I wonder why it was never instrumented anywhere 🤔
Since we are not using it, let's dump it to push towards an easier codebase and we can revert it if we need it in the future. We also need to ensure that our practices are in line with not allowing deletions, I seem to remember we've had some due to typos and such.
|
The linter failed, but otherwise lgtm |
|
/retest required |
|
@puerco: The
Use In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/test all |
|
it looks like it needs a rebase, weird that prow didn't catch it |
Weird that it's trying to fetch this branch from this repo and not my fork? |
These are only called by tests. This also hides a flag that was unused (for image size). Signed-off-by: Jon Johnson <jonjohnsonjr@gmail.com>
jonjohnsonjr
force-pushed
the
dead-checks
branch
from
5131426
to
cf4cd2a
Compare
|
what.is.this .. :| |
Ah yeah at the bottom. wtf. |
|
A revert to prow is going on here: kubernetes/test-infra#29167 |
|
/retest |
1 similar comment
|
/retest |
|
Great, in it goes! |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jonjohnsonjr, puerco 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 |
k8s-ci-robot
added
the
approved
These are only called by tests. This also hides a flag that was unused (for image size).
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
This removes a bunch of dead code to make it easier to understand what's going on in the promoter.
Which issue(s) this PR fixes:
None
Special notes for your reviewer:
I'm sorry.
Does this PR introduce a user-facing change?
/cc @puerco