Skip to content
This repository was archived by the owner on Dec 1, 2022. It is now read-only.

Fix pdb #1042

Merged
mgencur merged 2 commits into
openshift:mainfrom
skonto:fix_pdb_main
Feb 2, 2022
Merged

Fix pdb #1042
mgencur merged 2 commits into
openshift:mainfrom
skonto:fix_pdb_main

Conversation

@skonto
Copy link
Copy Markdown

@skonto skonto commented Feb 1, 2022

Right now there are failures in CI due to the wrong PDB version #1037. This was partially fixed in #1039.
Test that this patch works here: #1043

@skonto skonto requested review from mgencur, nak3 and rhuss February 1, 2022 13:46
@openshift-ci
Copy link
Copy Markdown

openshift-ci Bot commented Feb 1, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: skonto

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details 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 Feb 1, 2022
@rhuss
Copy link
Copy Markdown

rhuss commented Feb 1, 2022

Makes sense to me, but let @mgencur give it a second look, please. I wonder how we can collect such "temporary hacks" in a central document so that we remind ourselves to clean those up when e.g. OCP 4.12 arrives ?

@skonto
Copy link
Copy Markdown
Author

skonto commented Feb 1, 2022

@rhuss I can add this to such a doc (preferably commit it in this repo) but also want to consult @nak3 when he gets back if there are more hacks to document.
In general it would be nice to cleanup any TODO stuff from time to time. I am sure there are such comments here and there in the code base and at least the less urgent ones are part of the technical debt.

Comment thread openshift/e2e-common.sh Outdated
function create_configmaps(){
# Keep this until OCP 4.12.
# Check https://github.com/openshift/knative-serving/pull/1037#issuecomment-1021827931 for more.
sed -i -e "s|policy/v1|policy/v1beta1|g" openshift/release/knative-serving-ci.yaml
Copy link
Copy Markdown

@mgencur mgencur Feb 2, 2022

Choose a reason for hiding this comment

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

Looking at at update-to-head.sh script, it almost looks like this line should be moved after this line and then this little hack in tests would not be required. In other words: we already have the patch there so why not apply it before generating the knative-serving-ci.yaml ? I didn't try it but I think this should be the way to go.
Update: Moving the line there really propagates the patch into knative-serving-ci.yaml, tried that.

Copy link
Copy Markdown
Author

@skonto skonto Feb 2, 2022

Choose a reason for hiding this comment

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

Ok I see what you mean. We generate the ci yaml file when we do the file resolution in generate-release.sh. Why we applied the patches later then if we need them in the openshift/release/knative-serving-${release}.yaml file? I guess it is because this is the first time we have a patch in config and we didnt care about when patches would be applied before. Ok let me update the PR.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Yeah. Right, we need to apply the patch before calling generate-release.sh. I'm not sure why we had applying the patch later but it's probably the first time we're hitting this type of issue, as you mentioned.

@skonto
Copy link
Copy Markdown
Author

skonto commented Feb 2, 2022

@mgencur ready for review.

@mgencur
Copy link
Copy Markdown

mgencur commented Feb 2, 2022

Looks good to me. Merging

@mgencur mgencur merged commit 8e9a5c7 into openshift:main Feb 2, 2022
@mgencur
Copy link
Copy Markdown

mgencur commented Feb 2, 2022

Thanks!

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

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants