NO-ISSUE: Add tests-extension verify to openshift verify #439
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@tmshort: This pull request explicitly references no jira issue. 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
bot
added
the
approved
|
/lgtm |
|
Better if this updates the verify target of test-extension, but at least this will work. |
|
/hold for pending update |
openshift-ci
bot
added
the
do-not-merge/hold
|
/unhold |
openshift-ci
bot
removed
the
do-not-merge/hold
openshift/tests-extension/Makefile
Outdated
|
|
||
| .PHONY: update-metadata | ||
| update-metadata: #HELP Build and run 'update-metadata' to generate test metadata | ||
| update-metadata: build #HELP Build and run 'update-metadata' to generate test metadata |
There was a problem hiding this comment.
build is needed here to build the $(TOOLS_BIN_DIR)/olmv1-tests-ext executable used on the next line...
tmshort
force-pushed
the
update-verify
branch
2 times, most recently
from
4660978 to
4996826
Compare
tmshort
changed the title
|
Thanks. +1 this approach. I think the "perfect" form of this is where openshift/Makefile(verify) invokes |
| @echo "Cleaning metadata (removing codeLocations)..." | ||
| @jq 'map(del(.codeLocations))' $(METADATA) > $(METADATA).tmp && mv $(METADATA).tmp $(METADATA) | ||
|
|
||
| .PHONY: verify-metadata #HELP To verify that the metadata was properly update |
There was a problem hiding this comment.
Could we have a target specific for the verify-bindata?
Or change it to verify and update the helpers
openshift/tests-extension/Makefile
Outdated
| #SECTION Development | ||
| .PHONY: verify #HELP To verify the code | ||
| verify: tidy fmt vet lint | ||
| verify: bindata verify-metadata tidy fmt vet lint |
There was a problem hiding this comment.
To be useful in isolation, this needs to call git diff directly now that part of what it is doing is generating some data that we expect not to change.
There was a problem hiding this comment.
Then, when we run it, it will update the JSON with the tracked IDs of the tests
If we want to change the name, we need to delete the file and re-generate it.
The reason for not all be together was
-> https://github.com/openshift/operator-framework-operator-controller/blob/main/openshift/tests-extension/README.md#why-do-we-need-to-run-make-update-metadata
-> https://github.com/openshift/operator-framework-operator-controller/blob/main/openshift/tests-extension/README.md#development-workflow
|
/retest verify |
|
@tmshort: The The following commands are available to trigger optional jobs: 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-sigs/prow repository. |
|
/test verify |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: camilamacedo86, tmshort 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 |
|
/test verify |
| verify-metadata: update-metadata | ||
| @if ! git diff --exit-code $(METADATA); then \ | ||
| @if ! git diff --exit-code >&/dev/null; then \ | ||
| echo "ERROR: Metadata is out of date. Please run 'make build-update' and commit the result."; \ | ||
| exit 1; \ | ||
| fi |
There was a problem hiding this comment.
This feels like it will be confusing. We'll run update-metadata, but what if there's another diff somewhere else? This will still report "metadata out of date", right?
My two cents: drop verify-metadata. Now that verify updates the metadata and runs its own top-level git diff after running all generators, I think that's all we need.
But (and this is the dance I was talking about in Slack earlier), we'll need to:
- Merge this.
- Remove these lines (https://github.com/openshift/release/blob/0b2296f37754138ca4e6dca16c73c94a51a5dc55/ci-operator/config/openshift/operator-framework-operator-controller/openshift-operator-framework-operator-controller-main.yaml#L177-L178)
- Come back here and remove
verify-metadata
There was a problem hiding this comment.
We must verify metatada ( not matter how we call or from where )
if we move with it we can remove the whole above sanity test above
There was a problem hiding this comment.
I changed this around such that verify-metadata is no longer called.
Look above at verify
|
New changes are detected. LGTM label has been removed. |
tmshort
force-pushed
the
update-verify
branch
2 times, most recently
from
4e05fd6 to
b6aae7f
Compare
Signed-off-by: Todd Short <todd.short@me.com>
|
the verify build is continuing to fail due to |
|
@tmshort: The following test failed, say
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. |
5 participants
No description provided.