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

upgrade test: use docker.mirror.hashicorp.services to avoid docker login #17186

Merged
merged 2 commits into from
May 8, 2023

Conversation

huikang
Copy link
Collaborator

@huikang huikang commented Apr 28, 2023

Description

Use docker image from docker.mirror.hashicorp.services to avoid docker login in GHA

Also add image name as parameter to cluster.StandardUpgrade(utils.GetTargetImageName(), tc.targetVersion), because the image we upgrade to is consul:locally, which is different than the latest image docker.mirror.hashicorp.services/docker:1.15 for example.

PR Checklist

  • updated test coverage
  • external facing docs updated
  • appropriate backport labels added
  • not a security concern

@github-actions github-actions bot added theme/contributing Additions and enhancements to community contributing materials type/ci Relating to continuous integration (CI) tooling for testing or releases labels Apr 28, 2023
@huikang huikang force-pushed the upgrade-test-use-mirror-hashicorp branch from bbc2b3a to 6fa3fd6 Compare April 28, 2023 19:23
@huikang huikang requested review from loshz and a team April 28, 2023 19:24
@@ -324,7 +324,7 @@ jobs:
${{ matrix.test-cases }} \
--target-image ${{ env.CONSUL_LATEST_IMAGE_NAME }} \
--target-version local \
--latest-image ${{ env.CONSUL_LATEST_IMAGE_NAME }} \
--latest-image docker.mirror.hashicorp.services/${{ env.CONSUL_LATEST_IMAGE_NAME }} \
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@loshz , this is where we change the docker image to use docker.mirror.hashicorp.services. I am not sure where to remove the docker login since it seems needed to pull vault image. Please advise.

Copy link
Contributor

Choose a reason for hiding this comment

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

I pushed a change in 81e3c44

@huikang huikang added pr/no-changelog PR does not need a corresponding .changelog entry backport/1.15 This release series is no longer active on CE. Use backport/ent/1.15. labels Apr 28, 2023
@huikang
Copy link
Collaborator Author

huikang commented Apr 28, 2023

@loshz , we might just need to remove the docker login in CI: #17193

@huikang huikang force-pushed the upgrade-test-use-mirror-hashicorp branch 3 times, most recently from 696fecd to 3390837 Compare April 29, 2023 03:56
@loshz
Copy link
Contributor

loshz commented Apr 29, 2023

we might just need to remove the docker login in CI: #17193

If we’re removing the Dockerhub login, then we need to pull images from the HCP registry instead as we could be hit by quota limits if we have a lot of CI jobs running.

@huikang huikang force-pushed the upgrade-test-use-mirror-hashicorp branch from 3390837 to 377ca68 Compare May 1, 2023 14:03
@loshz
Copy link
Contributor

loshz commented May 2, 2023

The Docker changes LGTM, but I'm unsure if we need to add the image parameter in this PR? I'd like this to be as atomic as possible, so could we do that in a separate PR once we can validate the removal of Docker Hub in GHA works as expected?

@huikang
Copy link
Collaborator Author

huikang commented May 2, 2023

The Docker changes LGTM, but I'm unsure if we need to add the image parameter in this PR? I'd like this to be as atomic as possible, so could we do that in a separate PR once we can validate the removal of Docker Hub in GHA works as expected?

@loshz , are you referring to the changes in other files for upgrade test? If so, these changes are necessary since the latest image and target image name will be different:

  • Before: both uses consul as the image name
  • After: the latest image will be docker.mirror.hashicorp.services/consul:v1.15. Since the target image (the version to upgrade to) is built locally, the makefile uses the image name of consul:local. Without the change, the container uses the image from the previous image - docker.mirror.hashicorp.services/consul:local, which doesn't exist in this case.

func (c *Config) DockerImage() string {
return utils.DockerImage(c.Image, c.Version)
}

So I think these changes are needed for make CI passed.

@huikang huikang mentioned this pull request May 5, 2023
4 tasks
Copy link
Contributor

@loshz loshz left a comment

Choose a reason for hiding this comment

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

@huikang ah, I see! This LGTM - thanks for fixing the issue.

@loshz loshz mentioned this pull request May 8, 2023
4 tasks
@huikang
Copy link
Collaborator Author

huikang commented May 8, 2023

@loshz , actually your early suggestions make sense. So I split the change to upgrade test into a separate PR #17226. Given @NiniOak has approved the part in this one, could you help approve #17226?

After that, I can update this one to an atomic change in GHA config file. Thanks.

@loshz
Copy link
Contributor

loshz commented May 8, 2023

@loshz , actually your early suggestions make sense. So I split the change to upgrade test into a separate PR #17226. Given @NiniOak has approved the part in this one, could you help approve #17226?

After that, I can update this one to an atomic change in GHA config file. Thanks.

Ok, sounds good! So we're going to merge #17226 first and then this one?

@huikang
Copy link
Collaborator Author

huikang commented May 8, 2023

Ok, sounds good! So we're going to merge #17226 first and then this one?

Yes, that's correct. After merging #17226, the change in this PR will be downsized to only .github/workflows/test-integrations.yml.

@huikang huikang force-pushed the upgrade-test-use-mirror-hashicorp branch from 377ca68 to b86c1eb Compare May 8, 2023 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/1.15 This release series is no longer active on CE. Use backport/ent/1.15. pr/no-changelog PR does not need a corresponding .changelog entry theme/contributing Additions and enhancements to community contributing materials type/ci Relating to continuous integration (CI) tooling for testing or releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants