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

ksd: Bump manually to v0.0.7 #1475

Merged
merged 4 commits into from Dec 8, 2022
Merged

ksd: Bump manually to v0.0.7 #1475

merged 4 commits into from Dec 8, 2022

Conversation

oshoval
Copy link
Collaborator

@oshoval oshoval commented Dec 7, 2022 •
edited

What this PR does / why we need it:
Bump KubeSecondaryDNS manually (v0.0.7), since auto-bumper has problems.
See #1474

Beside that:

  • Install kubevirt before running KSD e2e tests.
  • Fix bump script (use set instead of update, because strings might be empty).

Special notes for your reviewer:

Release note:

None

@kubevirt-bot kubevirt-bot added release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Dec 7, 2022
@oshoval
Copy link
Collaborator Author

oshoval commented Dec 7, 2022 •
edited

/cc @AlonaKaplan @dteplits
FYI trying here v0.0.7 with the new logic and e2e test

@oshoval
Copy link
Collaborator Author

oshoval commented Dec 7, 2022

something need to be fixed about, will look into that

https://prow.ci.kubevirt.io/view/gs/kubevirt-prow/pr-logs/pull/kubevirt_cluster-network-addons-operator/1475/pull-e2e-cnao-kube-secondary-dns-functests/1600430201441882112

@oshoval oshoval marked this pull request as draft December 7, 2022 10:31
@kubevirt-bot kubevirt-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 7, 2022
@oshoval
ksd: Deploy kubevirt
4ebfec0 
Needed for the e2e tests

Signed-off-by: Or Shoval <oshoval@redhat.com>
@oshoval oshoval mentioned this pull request Dec 8, 2022
Closed
@oshoval oshoval force-pushed the bumpksd branch 2 times, most recently from c46d9d7 to c6f09f1 Compare December 8, 2022 09:14
@oshoval oshoval marked this pull request as ready for review December 8, 2022 09:14
@kubevirt-bot kubevirt-bot added size/S and removed size/XS do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Dec 8, 2022
@oshoval oshoval changed the title ksd: Bump manually ksd: Bump manually (and include e2e) Dec 8, 2022
@oshoval
Copy link
Collaborator Author

oshoval commented Dec 8, 2022 •
edited

Basically ready for review please
but must see https://prow.ci.kubevirt.io/view/gs/kubevirt-prow/pr-logs/pull/kubevirt_cluster-network-addons-operator/1475/pull-e2e-cnao-kube-secondary-dns-functests/1600780865955172352
passes (and that it includes the e2e test)

@oshoval
Copy link
Collaborator Author

oshoval commented Dec 8, 2022

lane passed (and includes the test)
Ready for review please

/cc @RamLavi

@AlonaKaplan
Copy link
Member

/lgtm

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Dec 8, 2022
@RamLavi
Copy link
Collaborator

RamLavi commented Dec 8, 2022

@oshoval please specify the version you are bumping to in the PR title and the relevant commits

Also, why do you mention "(and include e2e)" in the title? that lane is already running no, and including it is done by project-infra? can you explain what you mean?

@oshoval
Copy link
Collaborator Author

oshoval commented Dec 8, 2022 •
edited

Also, why do you mention "(and include e2e)" in the title? that lane is already running no, and including it is done by project-infra? can you explain what you mean?

There was a lane, but it just deployed KSD, and had stub e2e (echo hello world) on ksdKSDside (not on CNAO side)
an actual e2e tested was added on KSD v0.0.6 only

@oshoval oshoval changed the title ksd: Bump manually (and include e2e) ksd: Bump manually to v0.0.7 (and include e2e) Dec 8, 2022
@oshoval
ksd: Bump manually to v0.0.7
416508e 
Bump KubeSecondaryDNS manually, since auto-bumper has problems.
See kubevirt#1474

Signed-off-by: Or Shoval <oshoval@redhat.com>
@oshoval
ksd: Fix bump script
bfd053b 
Since the fields might have empty string,
need to use set instead of update.

Signed-off-by: Or Shoval <oshoval@redhat.com>
@oshoval
ksd: Run bump script (v0.0.7)
b98c936 
Signed-off-by: Or Shoval <oshoval@redhat.com>
@kubevirt-bot kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Dec 8, 2022
@oshoval
Copy link
Collaborator Author

oshoval commented Dec 8, 2022

@oshoval please specify the version you are bumping to in the PR title and the relevant commits

Done

@sonarcloud
Copy link

sonarcloud bot commented Dec 8, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@RamLavi
Copy link
Collaborator

RamLavi commented Dec 8, 2022

Also, why do you mention "(and include e2e)" in the title? that lane is already running no, and including it is done by project-infra? can you explain what you mean?

There was a lane, but it just deployed ksd, and had stub e2e (echo hello world) an actual e2e tested was added on v0.0.6 only

OK, but the bump PR usually doesn't detail what it includes/excludes.
Please mention only what is relevant to this repo (i.e. bump, fix script, etc..)

@oshoval oshoval changed the title ksd: Bump manually to v0.0.7 (and include e2e) ksd: Bump manually to v0.0.7 Dec 8, 2022
@oshoval
Copy link
Collaborator Author

oshoval commented Dec 8, 2022

OK, but the bump PR usually doesn't detail what it includes/excludes. Please mention only what is relevant to this repo (i.e. bump, fix script, etc..)

Done
Note that since this is the first PR that does include e2e i think it worth to mention it (but removed anyhow)

@oshoval
Copy link
Collaborator Author

oshoval commented Dec 8, 2022 •
edited

/cc @brianmcarey
FYI please, even 10 seconds here weren't enough for the podman socket.
Maybe CI is too busy ? or file descriptors reached max?
or just need some more time
https://prow.ci.kubevirt.io/view/gs/kubevirt-prow/pr-logs/pull/kubevirt_cluster-network-addons-operator/1475/pull-e2e-cnao-kube-secondary-dns-functests/1600817425589211136

/test pull-e2e-cnao-kube-secondary-dns-functests

(not related specifically to KSD)

@oshoval
Copy link
Collaborator Author

oshoval commented Dec 8, 2022

/test pull-e2e-cluster-network-addons-operator-ovs-cni-functests

not related

RamLavi
RamLavi approved these changes Dec 8, 2022
View reviewed changes
Copy link
Collaborator

@RamLavi RamLavi left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Dec 8, 2022
@kubevirt-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RamLavi

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

@kubevirt-bot kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 8, 2022
@kubevirt-bot kubevirt-bot merged commit b5ddc8a into kubevirt:main Dec 8, 2022
@RamLavi
Copy link
Collaborator

RamLavi commented Dec 8, 2022 •
edited

OK, but the bump PR usually doesn't detail what it includes/excludes. Please mention only what is relevant to this repo (i.e. bump, fix script, etc..)

Done Note that since this is the first PR that does include e2e i think it worth to mention it (but removed anyhow)

the action of adding the e2e was made on the ksd repo, hence it should be mentioned on the 0.0.7 release note. CNAO just deploys it. The fact that we run these e2e tests in tier1 scope is not relevant because the automation ksd script is already lade out and does not require changes in this PR..

@oshoval
Copy link
Collaborator Author

oshoval commented Dec 8, 2022

OK, but the bump PR usually doesn't detail what it includes/excludes. Please mention only what is relevant to this repo (i.e. bump, fix script, etc..)

Done Note that since this is the first PR that does include e2e i think it worth to mention it (but removed anyhow)

the action of adding the e2e was made on the ksd repo, hence it should be mentioned on the 0.0.7 release note. CNAO just deploys it. The fact that we run these e2e tests in tier1 scope is not relevant because the automation ksd script is already lade out and does not require changes in this PR..

It is not exact, as the first commit deployed kubevirt for the e2e tests
Anyhow no biggie
Thanks

@oshoval
Copy link
Collaborator Author

oshoval commented Dec 12, 2022

Can we create please a new CNAO version?
I will need one that will be consumed by HCO U/S
and includes the new KSD

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

@RamLavi RamLavi RamLavi approved these changes

@alonSadan alonSadan Awaiting requested review from alonSadan

@phoracek phoracek Awaiting requested review from phoracek

@AlonaKaplan AlonaKaplan Awaiting requested review from AlonaKaplan

@dteplits dteplits Awaiting requested review from dteplits

@qinqon qinqon Awaiting requested review from qinqon

@brianmcarey brianmcarey Awaiting requested review from brianmcarey

Assignees

@AlonaKaplan AlonaKaplan

@RamLavi RamLavi

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/S
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@oshoval @AlonaKaplan @RamLavi @kubevirt-bot