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 dryRun flag for backplane-cli #210

Merged
merged 3 commits into from
Sep 27, 2023

Conversation

xiaoyu74
Copy link
Contributor

@xiaoyu74 xiaoyu74 commented Sep 25, 2023

What type of PR is this?

feature

Which Jira/Github issue(s) does this PR fix?

Resolves https://issues.redhat.com/browse/OSD-18387

Special notes for your reviewer

  • Tested in my dev cluster with the dry-run flag and it works as expected to only show the JSON format of the test job without creating it.
  • The dry-run feature has been implemented in the backplane-api, so this PR is just to add dryRun flag to call the dryRun API.

Pre-checks (if applicable)

  • Validated the changes in a cluster
  • Included documentation changes with PR

@codecov-commenter
Copy link

codecov-commenter commented Sep 25, 2023

Codecov Report

Merging #210 (7b7cbad) into main (94dd041) will increase coverage by 0.36%.
Report is 2 commits behind head on main.
The diff coverage is 83.33%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #210      +/-   ##
==========================================
+ Coverage   51.53%   51.90%   +0.36%     
==========================================
  Files          51       51              
  Lines        3415     3447      +32     
==========================================
+ Hits         1760     1789      +29     
- Misses       1364     1366       +2     
- Partials      291      292       +1     
Files Coverage Δ
cmd/ocm-backplane/testJob/createTestJob.go 70.34% <83.33%> (+0.71%) ⬆️

... and 1 file with indirect coverage changes

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 25, 2023

@xiaoyu74: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/scan-optional 7b7cbad link false /test scan-optional

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

@bmeng
Copy link
Contributor

bmeng commented Sep 26, 2023

I think we should better not print the job id if it is a dry run?

$ ocm backplane2 testjob create -d
...
...

TestId: openshift-job-1695719774

@xiaoyu74
Copy link
Contributor Author

I think we should better not print the job id if it is a dry run?

$ ocm backplane2 testjob create -d
...
...

TestId: openshift-job-1695719774
if err != nil {
    utils.WriteJson(w, http.StatusInternalServerError, &utils.ErrorResponse{
        StatusCode: http.StatusInternalServerError,
        Message: fmt.Sprintf("Failed running dryrun for job on the cluster, error: %s", err),
    })
    return
}
utils.WriteJson(w, http.StatusOK, jobs.TestJobResult{
    TestId:  jobObjects.Pod.Name,
    Message: buf.String(),
    Status:  string(corev1.PodPending), // Always starts with Pending
})

@bmeng - Thanks for your review. I think it's just defined in the backplane API, if we think the TestId should not be printed, we can update in API code then but it's a separated issue from this PR.

@bmeng
Copy link
Contributor

bmeng commented Sep 27, 2023

I think we still can deal with it from https://github.com/openshift/backplane-cli/pull/210/files#diff-85bea5392ad379d93bf38a12b6a10a1b754877f9fd898d19ebaaf89c1a0f860fR184
But it is not a big deal, let's leave it as is.

@bmeng
Copy link
Contributor

bmeng commented Sep 27, 2023

/label tide/merge-method-squash
/lgtm

@openshift-ci openshift-ci bot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Sep 27, 2023
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 27, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 27, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bmeng, xiaoyu74

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 27, 2023
@openshift-merge-robot openshift-merge-robot merged commit 9d5f461 into openshift:main Sep 27, 2023
6 of 7 checks passed
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. lgtm Indicates that a PR is ready to be merged. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants