Skip to content

Conversation

periklis
Copy link
Contributor

@periklis periklis commented Mar 3, 2021

Description

This PR provides a minor fix for the elasticsearch-operator E2E upgrade suite. The suite was broken by changing the release schema from OCP to OpenShift Logging, i.e. switching from 4.6 to 5.x

/cc @vimalk78
/assign @jcantrill

Links

# Set this to 4.7 because we need to calculate the latest previous
# version prior to 5.0, which is 4.6 for OpenShift Logging.
major_version=4
minor_version=7
Copy link
Contributor

Choose a reason for hiding this comment

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

previous version should be 4.7 or 4.6?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previous to 5.0 is 4.6, but in order to calculate that via get_latests_previous_version in need to fake 5.0 as 4.7. The whole block should be removed in master.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok

Copy link
Contributor

@ewolinetz ewolinetz Mar 5, 2021

Choose a reason for hiding this comment

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

i think the fix should instead happen in the "get_latest_previous_version"
this feels like a hacky way to resolve this...

it also means that we are dependent on the 4.7 manifest being in our branch for this to work...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No we are not dependent to 4.7 manifest. We only pretend 5.0 to be 4.7 for the sake of this bash sourcery. The version 4.7 is used for calculation of 4.6 to create Subscription using operatorhub.

Copy link
Contributor

@vimalk78 vimalk78 left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 3, 2021
@periklis
Copy link
Contributor Author

periklis commented Mar 3, 2021

/retest

2 similar comments
@periklis
Copy link
Contributor Author

periklis commented Mar 3, 2021

/retest

@periklis
Copy link
Contributor Author

periklis commented Mar 4, 2021

/retest

@periklis periklis force-pushed the fix-eo-upgrade-tests branch from 4d9a14d to 45cbc4a Compare March 4, 2021 09:31
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 4, 2021
@periklis
Copy link
Contributor Author

periklis commented Mar 4, 2021

/retest

@vimalk78
Copy link
Contributor

vimalk78 commented Mar 5, 2021

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 5, 2021
Copy link
Contributor

@jcantrill jcantrill left a comment

Choose a reason for hiding this comment

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

/approve

@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jcantrill, periklis, vimalk78

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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 5, 2021
minor_version=7
fi

export version="$(echo $major_version.$minor_version)"
Copy link
Contributor

Choose a reason for hiding this comment

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

I see the export above the if block, but it serves the same purpose as it would if we exported it here after the if block

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No it is not! The export version should be set to 5.0 so that the upgrade bash sourcery can patch the subscription. However get_latest_previous_version needs the value 4.7 to calculate 4.6 as the previous version and in turn create the initial subscription.

@ewolinetz
Copy link
Contributor

/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 5, 2021
@periklis
Copy link
Contributor Author

periklis commented Mar 5, 2021

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 5, 2021
@ewolinetz
Copy link
Contributor

/hold

@periklis can you please address my questions comments?
in general if someone places a hold we should let that person remove it

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 5, 2021
@periklis
Copy link
Contributor Author

periklis commented Mar 5, 2021

/hold

@periklis can you please address my questions comments?
in general if someone places a hold we should let that person remove it

Addressed them! PTAL

@ewolinetz
Copy link
Contributor

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 5, 2021
@openshift-merge-robot openshift-merge-robot merged commit be2a4b1 into openshift:release-5.0 Mar 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants