Skip to content
This repository was archived by the owner on Feb 6, 2025. It is now read-only.

Use the Proper SLE15 SP2 JeOS GM image for testing on OpenStack #1200

Merged
merged 1 commit into from
Jun 30, 2020
Merged

Use the Proper SLE15 SP2 JeOS GM image for testing on OpenStack #1200

merged 1 commit into from
Jun 30, 2020

Conversation

dirkmueller
Copy link
Member

Why is this PR needed?

The business case this fixes is that the CI is using the final SP2 image that we ship to customers rather than something older and unsupported.

As such it brings more relevance to what we test and how we test and that is considered a good thing.

What does this PR do?

It fixes the Image configuration to point to the right image.

Anything else a reviewer needs to know?

There is no manual testing needed.

Info for QA

There is no QA needed. if CI passes its good.

Related info

This PR can be merged independently.

Status BEFORE applying the patch

The wrong image is used.

Status AFTER applying the patch

The SLE15 SP2 GM JeOS image for OpenStack is being used for OpenStack testing.

Docs

The docs need to refer to the same image.

Merge restrictions

(Please do not edit this)

We are in v4-maintenance phase, so we will restrict what can be merged to prevent unexpected surprises:

What can be merged (merge criteria):
    2 approvals:
        1 developer: code is fine
        1 QA: QA is fine
    there is a PR for updating documentation (or a statement that this is not needed)

@evrardjp
Copy link
Contributor

I am already using those images (but not the reboot fix), and that works for me. 👍

@evrardjp
Copy link
Contributor

I think this should be placed on Z squad's radar. What do you think @jordimassaguerpla ?

Copy link
Contributor

@pablochacin pablochacin left a comment

Choose a reason for hiding this comment

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

There seem to be two unrelated changes in the PR: the image and the reboot logic. If you drop the second commit from this PR we can take the update of the image and test it in the CI.

The reboot logic, we prefer not to changed in the SAME PR unless it is broken, which is not the case as far as we know and JP reports.

@dirkmueller
Copy link
Member Author

@pablochacin I removed the 2nd commit. I already tested it, and so did JP. I think a review would be enough.

@dirkmueller dirkmueller requested a review from pablochacin June 29, 2020 15:50
evrardjp
evrardjp previously approved these changes Jun 29, 2020
Copy link
Contributor

@evrardjp evrardjp left a comment

Choose a reason for hiding this comment

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

That's what I am using.

@evrardjp
Copy link
Contributor

Interesting that the pr-head chokes on a id_shared file... Should we edit the authorized_keys var to add the appropriate ci pubkey into ci/infra/openstack/terraform.tfvars.json.ci.example ? I suppose there is something else in our CI tooling dealing with this...

@pablochacin
Copy link
Contributor

Interesting that the pr-head chokes on a id_shared file... Should we edit the authorized_keys var to add the appropriate ci pubkey into ci/infra/openstack/terraform.tfvars.json.ci.example ? I suppose there is something else in our CI tooling dealing with this...

I observed this same error in PR #1205 The problem seems to be related to the changes in PR #1168 (the location of files change) I'm looking at it.

@dirkmueller
Copy link
Member Author

potentially this required a rebase. I've rebased on top of master.

@dirkmueller dirkmueller reopened this Jun 29, 2020
@pablochacin
Copy link
Contributor

I run an e2e test job with the PR and the cluster was deployed correctly. The deployment failed with an unrelated issue related to the image repository. So from my perspective, the new image solves the ssh issue.

I'm still investigating why the PR tests are failing. It also seems to be unrelated, as I mentioned in a previous comment.

@pablochacin
Copy link
Contributor

I'm still investigating why the PR tests are failing. It also seems to be unrelated, as I mentioned in a previous comment.

Effectively, it is due to a problem with PR #1168

Copy link
Contributor

@pablochacin pablochacin left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@evrardjp evrardjp left a comment

Choose a reason for hiding this comment

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

It's weird we provide image_name in L4 instead of L3 on the example file, but it's good enough for me.

@kkaempf
Copy link
Member

kkaempf commented Jun 30, 2020

@jordimassaguerpla jordimassaguerpla merged commit ba48dff into SUSE:master Jun 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants