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

Fix e2e tests #292

Merged
merged 1 commit into from
Sep 15, 2021
Merged

Fix e2e tests #292

merged 1 commit into from
Sep 15, 2021

Conversation

furkatgofurov7
Copy link
Member

@furkatgofurov7 furkatgofurov7 commented Sep 10, 2021 •
edited
Loading

What this PR does / why we need it:
Fixes e2e tests by:

  • removing unnecessary RunAsGroup=true field to keep up a consistency with m3-dev-env templates
  • adding a workaround for nameprefix issue in IPAM kustomization

Update: co-authored by: @namnx228 (nam.xuan.nguyen@est.tech)

@metal3-io-bot metal3-io-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 10, 2021
@furkatgofurov7
Copy link
Member Author

/test-v1a5-e2e

@namnx228
Copy link
Member

/test-v1a5-centos-e2e

@furkatgofurov7
Copy link
Member Author

/test-v1a5-e2e

@furkatgofurov7
Copy link
Member Author

/test-v1a5-e2e

@mboukhalfa
Copy link
Member

/test-v1a5-centos-e2e

@furkatgofurov7
Copy link
Member Author

/test-v1a5-e2e

1 similar comment
@namnx228
Copy link
Member

/test-v1a5-e2e

@namnx228
Copy link
Member

/test-v1a5-e2e

@namnx228
Copy link
Member

/test-v1a5-e2e

@namnx228
Copy link
Member

/test-v1a5-e2e

@namnx228
Copy link
Member

/test-v1a5-centos-e2e

@namnx228
Copy link
Member

/test-v1a5-e2e
/test-v1a5-centos-e2e
/test-integration

@Rozzii
Copy link
Member

Rozzii commented Sep 13, 2021

LGTM

@metal3-io-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mboukhalfa, Rozzii
To complete the pull request process, please ask for approval from furkatgofurov7 after the PR has been reviewed.

The full list of commands accepted by this bot can be found 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

@furkatgofurov7
Copy link
Member Author

/retitle Fix e2e tests
/test-v1a5-e2e
/test-v1a5-centos-e2e
/test-integration

@metal3-io-bot metal3-io-bot changed the title WIP: Fix e2e tests Fix e2e tests Sep 13, 2021
@metal3-io-bot metal3-io-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 13, 2021
@namnx228
Copy link
Member

/lgtm

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Sep 13, 2021
kashifest
kashifest reviewed Sep 14, 2021
View reviewed changes
Copy link
Member

@kashifest kashifest left a comment

Choose a reason for hiding this comment

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

I am not sure about this quick fix since we know the proper fix would be to use the override layer and same provider components specially since we have other overrides in that yaml as well , like image overrides.

test/e2e/e2e_test.go Outdated
org := "ipam-serving-cert"
dst := "capm3-ipam-serving-cert"
component = []byte(strings.Replace(string(component), org, dst, -1))
component = []byte(strings.Replace(string(component), "capm3-capm3", "capm3", -1))
Copy link
Member

Choose a reason for hiding this comment

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

why this line is needed?

Copy link
Member

Choose a reason for hiding this comment

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

The line 101 replaces ipam-serving-cert by capm3-ipam-serving-cert, and it also replaces capm3-ipam-serving-cert by capm3-capm3-ipam-serving-cert. The line 102 fixes this issue.

Copy link
Member

@namnx228 namnx228 Sep 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The proper fix needs time to be ready, so this quick fix can buy more time, helping other e2e test PRs to go in. We can remove it later.

Copy link
Member

Choose a reason for hiding this comment

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

The line 101 replaces ipam-serving-cert by capm3-ipam-serving-cert, and it also replaces capm3-ipam-serving-cert by capm3-capm3-ipam-serving-cert. The line 102 fixes this issue.

For that reason, I included the namespace also in my fix in m3-dev-env like this capm3-system/ipam-serving-cert to capm3-system/capm3-ipam-serving-cert see here

@kashifest
Copy link
Member

/approve

@namnx228
Copy link
Member

/test-v1a5-e2e
/test-v1a5-centos-e2e
/test-integration

@kashifest
Copy link
Member

Thanks @namnx228 it looks good now, by the way @namnx228 @furkatgofurov7 can you squash the commits and make the commit message more informative ?

@namnx228
Fix the e2e issues related to the uplift k8s to v1.22 .0
69e32b5 
This commits solves two issues:
-  Removing unnecessary RunAsGroup=true field to keep up a consistency with m3-dev-env templates
 - Adding a workaround for nameprefix issue in IPAM kustomization
Co-authored-by: furkatgofurov7 <furkat.gofurov@est.tech>
@namnx228
Copy link
Member

/test-v1a5-e2e
/test-v1a5-centos-e2e
/test-integration

@fmuyassarov
Copy link
Member

Since prow is down right now I am going to merge this PR manually. It has passed all the required CI checks and got approve and lgtm.

@fmuyassarov fmuyassarov merged commit 4bfcdef into metal3-io:master Sep 15, 2021
@huutomerkki huutomerkki mentioned this pull request Feb 19, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@namnx228 namnx228 namnx228 left review comments

@kashifest kashifest kashifest left review comments

@mboukhalfa mboukhalfa mboukhalfa approved these changes

@Rozzii Rozzii Rozzii approved these changes

Assignees
No one assigned
Labels
lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@furkatgofurov7 @namnx228 @mboukhalfa @Rozzii @metal3-io-bot @kashifest @fmuyassarov