OCPBUGS-38499: Fix integration tests #8789
Conversation
pawanpinjarkar
force-pushed
the
fix-integration-tests
branch
2 times, most recently
from
944ed83 to
89b17c4
Compare
|
/test |
|
/test ? |
|
/test integration-tests |
|
@pawanpinjarkar: The following commands are available to trigger required jobs:
The following commands are available to trigger optional jobs:
Use
DetailsIn 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-sigs/prow repository. |
|
@pawanpinjarkar: This pull request explicitly references no jira issue. DetailsIn 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/valid-reference
|
/test integration-tests |
1 similar comment
|
/test integration-tests |
|
/retest-required |
|
/test integration-tests |
pawanpinjarkar
force-pushed
the
fix-integration-tests
branch
from
397cbbe to
9b1a791
Compare
|
/test integration-tests |
2 similar comments
|
/test integration-tests |
|
/test integration-tests |
|
/retest-required |
|
/test integration-tests |
|
/cc @bfournie |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sadasu The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
There was a problem hiding this comment.
Should we move this down to line 296 so we are only skipping the byteCompare() (i.e. still check that the file exists in the place we expect it)?
Even then though, looking at some of the actual tests, we pass a pull secret like:
pullSecret: '{"auths": {"quay.io": {"auth": "c3VwZXItc2VjcmV0Cg=="}}}'
and in the output expect something like:
apiVersion: v1
kind: Secret
metadata:
creationTimestamp: null
name: ostest-pull-secret
namespace: cluster0
stringData:
.dockerconfigjson: |-
{
"auths": {
"quay.io": {
"auth": "c3VwZXItc2VjcmV0Cg=="
}
}
}
so there's a lot we are missing by not checking the content.
Maybe we could take a similar approach to the release image and do something like:
pullSecret: '{"auths": {"quay.io": {"auth": "$QUAY_PULL_SECRET_AUTH"}}, {"ci.openshift.org": {"auth": "$OPENSHIFTCI_PULL_SECRET_AUTH"}}}'
(note - I don't actually know what the pull secret we get in CI looks like or how much of it we need.)
Then we could just set the values in the environment, wouldn't have to load-and-save the install-config, and could verify the full output format.
There was a problem hiding this comment.
So the case you were referring, for example, is https://github.com/openshift/installer/blob/master/cmd/openshift-install/testdata/agent/image/manifests/default_manifests.txt but here we are comparing the pull-secret with its contents correctly, isn't it? e.g. in
/etc/assisted/manifests/pull-secret.yaml against expected/pull-secret.yaml ?
There was a problem hiding this comment.
Sample CI pull-secret looks like this -
{
"auths": {
"image-registry.openshift-image-registry.svc.cluster.local:5000": {
"auth": "$A"
},
"image-registry.openshift-image-registry.svc:5000": {
"auth": "$B"
},
"qci-pull-through-cache-us-east-1-ci.apps.ci.l2s4.p1.openshiftapps.com": {
"auth": "$C"
},
"quay-proxy.ci.openshift.org": {
"auth": "$C"
},
"quay.io": {
"auth": "$D",
"email": "[email protected]"
},
"quay.io/openshift/ci": {
"auth": "$C"
},
"registry.build05.ci.openshift.org": {
"auth": "$E"
},
"registry.ci.openshift.org": {
"auth": "$F"
}
}
}
The email is the actual one, so we'll need to handle it carefully in the code to avoid exposing it. However, I'm struggling to understand the advantage of using the real pull secret and embedding it in the actual and expected test data. Currently, we're already validating the content of the pull secret effectively.
There was a problem hiding this comment.
Yeah, replacing all of that would be messy.
Presumably the only thing we use the pull secret for in the integration tests is to grab the ISO from the release image? If it turned out that a pull secret with only a couple of these auths was sufficient, then that starts to look more feasible.
So the case you were referring, for example, is https://github.com/openshift/installer/blob/master/cmd/openshift-install/testdata/agent/image/manifests/default_manifests.txt but here we are comparing the pull-secret with its contents correctly, isn't it? e.g. in
installer/cmd/openshift-install/testdata/agent/image/manifests/default_manifests.txt
Line 11 in 275af57
isocmp agent.x86_64.iso /etc/assisted/manifests/pull-secret.yaml expected/pull-secret.yaml
we are checking the contents of /etc/assisted/manifests/pull-secret.yaml against expected/pull-secret.yaml ?
We were, but this change to isoCmp() means that we will no longer actually compare the contents in CI. So if any of the formatting around the pull secret changes, we will not be able to detect it.
There was a problem hiding this comment.
So if any of the formatting around the pull secret changes, we will not be able to detect it.
The e2e jobs will fail in those case
|
/lgtm |
|
/test integration-tests |
2 similar comments
|
/test integration-tests |
|
/test integration-tests |
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
pawanpinjarkar
force-pushed
the
fix-integration-tests
branch
from
990c87f to
d7a69e4
Compare
pawanpinjarkar
force-pushed
the
fix-integration-tests
branch
from
d7a69e4 to
2a4d3f3
Compare
|
Thanks @pawanpinjarkar latest changes look fine to me. Can you please fix the linting issues (and possibly squash the commits)? |
Update the pull secret in the expected install-config file when AUTH_FILE is set. Check if the image files contain the pull-secret.yaml file. Use $OPENSHIFT_INSTALL_RELEASE_IMAGE_OVERRIDE to replace test data and pass it to the test script environment. This is typically set in a CI job to reference the ephemeral payload release. Skip errors when the test does not require reading install-config.yaml.
pawanpinjarkar
force-pushed
the
fix-integration-tests
branch
from
f4d9a09 to
ca075d2
Compare
|
/retest |
|
@pawanpinjarkar: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
|
/jira refresh |
|
@pawanpinjarkar: This pull request references Jira Issue OCPBUGS-38499, which is invalid:
Comment DetailsIn 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@pawanpinjarkar: This pull request references Jira Issue OCPBUGS-38499, which is valid. 3 validation(s) were run on this bug
DetailsIn 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/lgtm |
|
/cherrypick release-4.17 |
|
@pawanpinjarkar: once the present PR merges, I will cherry-pick it on top of release-4.17 in a new PR and assign it to you. DetailsIn 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-sigs/prow repository. |
|
@pawanpinjarkar: Jira Issue OCPBUGS-38499: All pull requests linked via external trackers have merged: Jira Issue OCPBUGS-38499 has been moved to the MODIFIED state. DetailsIn 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 openshift-eng/jira-lifecycle-plugin repository. |
|
@pawanpinjarkar: new pull request created: #9018 DetailsIn 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-sigs/prow repository. |
7 participants
Uh oh!
There was an error while loading. Please reload this page.