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.

Sign up for GitHub

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

Jump to bottom

HOSTEDCP-1542: cmd: add an option to render into a file, use it in e2e #4036

Merged
merged 3 commits into from
Jun 4, 2024
Merged

HOSTEDCP-1542: cmd: add an option to render into a file, use it in e2e #4036

merged 3 commits into from
Jun 4, 2024

Conversation

stevekuznetsov
Copy link
Contributor

The closest thing we have to testing the command-line is our end-to-end code that re-uses the options structs and runs them through the logic that the command-line does. We can capture a lot of the work done in the command-line by inspecting the output manifests, which is easiest to do by rendering them as a test artfact.

Helps to test #4018

@openshift-ci-robot
Copy link

openshift-ci-robot commented May 15, 2024 •
edited by openshift-ci bot

@stevekuznetsov: This pull request references HOSTEDCP-1542 which is a valid jira issue.

In response to this:

The closest thing we have to testing the command-line is our end-to-end code that re-uses the options structs and runs them through the logic that the command-line does. We can capture a lot of the work done in the command-line by inspecting the output manifests, which is easiest to do by rendering them as a test artfact.

Helps to test #4018

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 openshift-eng/jira-lifecycle-plugin repository.

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label May 15, 2024
@openshift-ci openshift-ci bot added area/cli Indicates the PR includes changes for CLI area/testing Indicates the PR includes changes for e2e testing and removed do-not-merge/needs-area labels May 15, 2024
bryan-cox
bryan-cox reviewed May 15, 2024
View reviewed changes
Copy link
Member

@bryan-cox bryan-cox left a comment

Choose a reason for hiding this comment

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

lgtm, just had a few very minor nits

cmd/cluster/cluster.go Outdated Show resolved Hide resolved
product-cli/cmd/cluster/cluster.go Outdated Show resolved Hide resolved
@bryan-cox
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 15, 2024
@bryan-cox
Copy link
Member

/retest-required

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label May 16, 2024
@muraee
Copy link
Contributor

muraee commented May 16, 2024

why do we even need this? all resources created by the cli are already part of the dump. What is the added value of having them in a single manifest. You will mainly be looking only at the HostedCluster / NodePool resources, which are easily found in the dump,

@stevekuznetsov
Copy link
Contributor Author

why do we even need this?

I wrote a PR that changes how the CLI works. I want to test that, and to do that I wanted to capture what the CLI was going to do by dumping the objects it would make, then we can assert over that output. I don't want post-creation modifications, etc, to influence this. Can you think of another way to capture it?

@muraee
Copy link
Contributor

muraee commented May 16, 2024

I don't want post-creation modifications, etc, to influence this
the HO wouldn't modify the spec of the HostedCluster or NodePool, and the API defaults would only apply if the fields are not set by the CLI. So we would know if the CLI did something wrong.

@stevekuznetsov
Copy link
Contributor Author

@muraee I want to test the CLI. While it's possible that nothing touches these manifests after the fact, having to a) know what the list of manifests is, b) collect them from the larger artifact dir, c) know which fields to ignore and when is much more difficult than simply asserting over the output of the CLI.

@csrwng
Copy link
Contributor

csrwng commented May 20, 2024

/approve

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented May 20, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: csrwng, stevekuznetsov

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 May 20, 2024
muraee
muraee reviewed May 21, 2024
View reviewed changes
test/e2e/util/fixture.go Outdated Show resolved Hide resolved
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 23, 2024
@stevekuznetsov
cmd: add an option to render into a file, use it in e2e
78b8349 
The closest thing we have to testing the command-line is our end-to-end
code that re-uses the options structs and runs them through the logic
that the command-line does. We can capture a lot of the work done in the
command-line by inspecting the output manifests, which is easiest to do
by rendering them as a test artfact.

Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>
@stevekuznetsov
test/e2e: checkpoint the output of cluster creation
dbbdfe0 
Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>
@stevekuznetsov
test/e2e: resolve dump dir once
eef4a0a 
Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>
@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 6fea573 and 0 for PR HEAD eef4a0a in total

@openshift-ci-robot
Copy link

/hold

Revision eef4a0a was retested 3 times: holding

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 31, 2024
@bryan-cox
Copy link
Member

/unhold

@bryan-cox
Copy link
Member

/retest

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 31, 2024
@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 510acd1 and 2 for PR HEAD eef4a0a in total

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD f4b077c and 1 for PR HEAD eef4a0a in total

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD afd17d6 and 0 for PR HEAD eef4a0a in total

@openshift-ci-robot
Copy link

/hold

Revision eef4a0a was retested 3 times: holding

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 1, 2024
@stevekuznetsov
Copy link
Contributor Author

/retest

@bryan-cox
Copy link
Member

/unhold

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 3, 2024
@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 94174f5 and 2 for PR HEAD eef4a0a in total

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD c0c55a0 and 1 for PR HEAD eef4a0a in total

@stevekuznetsov
Copy link
Contributor Author 

   * could not run steps: step [input:root] failed: failed to wait for importing imagestreamtags on ci-op-lbq4w3i0/pipeline: failed to reimport the tag ci-op-lbq4w3i0/pipeline:ocp_4.16_base-rhel9: unable to import tag ci-op-lbq4w3i0/pipeline:ocp_4.16_base-rhel9 with message Internal error occurred: [dockerimage.image.openshift.io "quay.io/openshift/ci:ci-op-lbq4w3i0_pipeline_ocp_4.16_base-rhel9" not found, dockerimage.image.openshift.io "quay-proxy.ci.openshift.org/openshift/ci:ci-op-lbq4w3i0_pipeline_ocp_4.16_base-rhel9" not found] on the image stream even after (6) imports: timed out waiting for the condition

/retest

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 7188a4c and 0 for PR HEAD eef4a0a in total

@openshift-ci-robot
Copy link

/hold

Revision eef4a0a was retested 3 times: holding

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 4, 2024
@bryan-cox
Copy link
Member

/unhold

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 4, 2024
@bryan-cox
Copy link
Member

/retest-required

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD cd2d529 and 2 for PR HEAD eef4a0a in total

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Jun 4, 2024 •
edited

@stevekuznetsov: 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/e2e-azure eef4a0a link false /test e2e-azure

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

@bryan-cox
Copy link
Member

/test e2e-kubevirt-aws-ovn

@openshift-merge-bot openshift-merge-bot bot merged commit e66add8 into openshift:main Jun 4, 2024
14 of 15 checks passed
@stevekuznetsov
Copy link
Contributor Author

holy moly we did it

stevekuznetsov added a commit to stevekuznetsov/hypershift that referenced this pull request Jun 4, 2024
@stevekuznetsov
Revert "Merge pull request openshift#4036 from stevekuznetsov/skuznet…
d27d21a 
…s/expose-cli-output"

This reverts commit e66add8, reversing
changes made to cd2d529.

Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>
@openshift-bot
Copy link

[ART PR BUILD NOTIFIER]

This PR has been included in build ose-hypershift-container-v4.17.0-202406041942.p0.ge66add8.assembly.stream.el9 for distgit hypershift.
All builds following this will include this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@muraee muraee muraee left review comments

@bryan-cox bryan-cox bryan-cox left review comments

@davidvossel davidvossel Awaiting requested review from davidvossel

@sjenning sjenning Awaiting requested review from sjenning

Assignees

@muraee muraee

@bryan-cox bryan-cox

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/cli Indicates the PR includes changes for CLI area/testing Indicates the PR includes changes for e2e testing jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@stevekuznetsov @openshift-ci-robot @bryan-cox @muraee @csrwng @openshift-bot @openshift-merge-robot