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

add canonical service controller to 1.6 #123

Merged
merged 3 commits into from
Aug 21, 2020

Conversation

PatchworkGuilt
Copy link
Collaborator

As we outlined in go/canonical-service-asm-build, this adds the canonical service controller to the ASM distro release.

To save your time during review: this is the exact same controller.yaml file copy/pasted across 4 folders.

@elfinhe
Copy link
Collaborator

elfinhe commented Aug 20, 2020

How do we test canonical services? Or it is done manually?

@PatchworkGuilt
Copy link
Collaborator Author

@elfinhe Integration tests are run as part of the image build process. They can be found here: https://team.git.corp.google.com/csm-ux-eng/canonical-service/+/refs/heads/master/controllers/

Copy link
Collaborator

@elfinhe elfinhe left a comment

Choose a reason for hiding this comment

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

Some general comments about folder structure:

  1. in asm/ asm-citadel/: just move canonical-service 1 level up. cluster is used for defining cluster infra.
  2. Also need to update asm-patch/resources/kustomization.yaml and asm-patch-citadel/resources/kustomization.yaml to include canonical into the resources of kustomize.

@PatchworkGuilt
Copy link
Collaborator Author

Thank you, I've cleaned up the folder structure and added it to those two kustomization.yaml files.

Copy link
Contributor

@yangminzhu yangminzhu left a comment

Choose a reason for hiding this comment

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

Mostly looks good, only some small comments, thanks!

asm/Kptfile Outdated Show resolved Hide resolved
asm-citadel/cluster/istio-operator.yaml Outdated Show resolved Hide resolved
asm-citadel/cluster/cluster.yaml Outdated Show resolved Hide resolved
Copy link
Collaborator

@elfinhe elfinhe left a comment

Choose a reason for hiding this comment

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

LGTM! but the latest commit mentioned my commit, which commit is that?

@PatchworkGuilt
Copy link
Collaborator Author

Unclear how, but somehow my attempts to rebase my branch to include the latest changes you made to 'release-1.60-asm' wound up including all your changes in my branch, which made a huge mess (https://github.com/GoogleCloudPlatform/anthos-service-mesh-packages/tree/release-1.6-asm). I did a 'git reset' and rebuilt my changes fresh to bail myself out.

Copy link
Contributor

@yangminzhu yangminzhu left a comment

Choose a reason for hiding this comment

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

thank you!

@PatchworkGuilt PatchworkGuilt merged commit e1c5635 into release-1.6-asm Aug 21, 2020
@michaelsun3
Copy link

what is the canonical-service-controller for?

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

Successfully merging this pull request may close these issues.

4 participants