Skip to content

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

Merged
huikang merged 2 commits intomainfrom
upgrade-test-use-mirror-hashicorp
May 8, 2023
Merged

upgrade test: use docker.mirror.hashicorp.services to avoid docker login#17186
huikang merged 2 commits intomainfrom
upgrade-test-use-mirror-hashicorp

Conversation

@huikang
Copy link
Copy Markdown
Collaborator

@huikang huikang commented Apr 28, 2023 •
edited
Loading

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 a team and loshz April 28, 2023 19:24
huikang
huikang commented Apr 28, 2023
View reviewed changes
.github/workflows/test-integrations.yml
--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
Copy Markdown
Collaborator Author

@huikang huikang Apr 28, 2023

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
Copy Markdown
Contributor

@loshz loshz Apr 28, 2023

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
Copy Markdown
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
Copy Markdown
Contributor

loshz commented Apr 29, 2023 •
edited
Loading

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 mentioned this pull request Apr 29, 2023
Merged
4 tasks
@huikang huikang force-pushed the upgrade-test-use-mirror-hashicorp branch from 3390837 to 377ca68 Compare May 1, 2023 14:03
@loshz
Copy link
Copy Markdown
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
Copy Markdown
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.

consul/test/integration/consul-container/libs/cluster/agent.go

Lines 73 to 75 in 90fc9bd

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
Merged
4 tasks
loshz
loshz approved these changes May 8, 2023
View reviewed changes
Copy link
Copy Markdown
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
Closed
4 tasks
@huikang
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Collaborator Author

huikang commented May 8, 2023 •
edited
Loading

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.

This was referenced May 8, 2023
Closed
Closed
Merged
@huikang huikang force-pushed the upgrade-test-use-mirror-hashicorp branch from 377ca68 to b86c1eb Compare May 8, 2023 16:04
@huikang huikang merged commit db253b6 into main May 8, 2023
@huikang huikang deleted the upgrade-test-use-mirror-hashicorp branch May 8, 2023 17:15
@hc-github-team-consul-core hc-github-team-consul-core mentioned this pull request May 8, 2023
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@NiniOak NiniOak NiniOak approved these changes

+1 more reviewer

@loshz loshz loshz approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@huikang @loshz @NiniOak