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.

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

e2e test with installer #313

Merged
merged 1 commit into from
Jan 17, 2022
Merged

Conversation

anusha94
Copy link
Contributor

@anusha94 anusha94 commented Jan 13, 2022

What this PR does / why we need it:

  • removed the skip-installation flag from e2e test
  • updated test/e2e/BYOHDockerfile to exclude k8s components
  • added a few additional required packages (ufw and dbus) to the Dockerfile
  • added a new dev-dockerfile to continue having the same experience in
    getting started and local-dev guides

Successful e2e run: https://github.com/vmware-tanzu/cluster-api-provider-bringyourownhost/runs/4800081237?check_suite_focus=true

Notes for Reviewer:
Adding additional Dockerfile so that the current getting started guides are not affected by this change. This PR's aim is to enable running e2e tests in GH Actions without skipping the installer workflow. Currently, the getting started guides skip this step and we want to continue doing that until we figure out a better way to not modify host config or some other alternative.

@anusha94 anusha94 marked this pull request as draft January 13, 2022 07:09
@anusha94 anusha94 changed the title E2e installer e2e test with installer Jan 13, 2022
- removed the skip-installation flag from e2e test
- updated test/e2e/BYOHDockerfile to exclude k8s components
- added a few additional required packages to the Dockerfile
- added a new dev-dockerfile to continue having the same experience in
  getting started and local-dev guides
@anusha94 anusha94 marked this pull request as ready for review January 13, 2022 07:51
@anusha94
Copy link
Contributor Author

@jamiemonserrate In the e2e test, we don't have to start the agent with sudo because the container itself it started with privileged = true

&container.HostConfig{Privileged: true,

@jamiemonserrate @dharmjit
wdyt of the contents of BYOHDockerfile? Currently I've removed containerd and other k8s bits. Anything else that the installer is taking care of and shouldn't be present in the Dockerfile?

Copy link
Contributor

@jamiemonserrate jamiemonserrate left a comment

Choose a reason for hiding this comment

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

Reviewed. This LGTM.

We still have the issue that the tests will change kernel settings for users who attempt to run these tests locally. But lets fix that in a subsequent PR.

Copy link
Contributor

@dharmjit dharmjit left a comment

Choose a reason for hiding this comment

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

This looks good to me.

Nitpick only, Is there some other dir to keep BYOHDockerFileDev. Do you thinkBYOHDevDockerFile is a better name.

@anusha94
Copy link
Contributor Author

This looks good to me.

Nitpick only, Is there some other dir to keep BYOHDockerFileDev. Do you thinkBYOHDevDockerFile is a better name.

Yea I can change the filename. Where do you think we can keep this file? I kept in the docs folder as it is being used there

@dharmjit
Copy link
Contributor

Where do you think we can keep this file?

My initial thoughts were docs should only contain documentation and not code. Do you think root dir could be an alternate?

@anusha94 anusha94 merged commit 072890a into vmware-tanzu:main Jan 17, 2022
@anusha94
Copy link
Contributor Author

My initial thoughts were docs should only contain documentation and not code. Do you think root dir could be an alternate?

Will do this separately as part of a different PR. Merged this so that other PRs can benefit from running a full e2e test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants