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

Admiralty integration tutorial #213

Merged
merged 3 commits into from
Dec 16, 2020
Merged

Admiralty integration tutorial #213

merged 3 commits into from
Dec 16, 2020

Conversation

ytsarev
Copy link
Member

@ytsarev ytsarev commented Dec 7, 2020

Provides short demo local setup of k8gb working
together with admiralty.io

Provides short demo local setup of k8gb working
together with admiralty.io
donovanmuller
donovanmuller previously approved these changes Dec 7, 2020
kuritka
kuritka previously approved these changes Dec 7, 2020
k0da
k0da previously approved these changes Dec 7, 2020
Copy link
Collaborator

@k0da k0da left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@somaritane somaritane left a comment

Choose a reason for hiding this comment

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

lgtm, just several small fixes ahead.

...
```

Observe that pods were scheduled to Target cluster after then Annotation patch
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion just in order to be consistent with beginning of tutorial, 'target' is used with small caps there.

Suggested change
Observe that pods were scheduled to Target cluster after then Annotation patch
Observe that pods were scheduled to the target clusters after the application of Annotation patch.

Copy link
Member Author

Choose a reason for hiding this comment

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

or maybe vice versa for capital? As for

apiVersion: multicluster.admiralty.io/v1alpha1
kind: Target

Copy link
Contributor

Choose a reason for hiding this comment

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

sure thing, just needs to be capital everywhere for consistency 👍


## Add pod annotations to enable multi-cluster scheduling

Edit `podinfo` Deployment to add admiralty `multicluster.admiralty.io/elect` scheduling Annotation to Pod spec template
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Edit `podinfo` Deployment to add admiralty `multicluster.admiralty.io/elect` scheduling Annotation to Pod spec template
Edit `podinfo` Deployment to add Admiralty `multicluster.admiralty.io/elect` scheduling Annotation to Pod spec template

@ytsarev ytsarev dismissed stale reviews from k0da, kuritka, and donovanmuller via d42937c December 7, 2020 15:47
@ytsarev ytsarev requested a review from somaritane December 7, 2020 15:51
@ytsarev
Copy link
Member Author

ytsarev commented Dec 7, 2020

@somaritane thanks a ton for the thorough review, please check again

Copy link
Contributor

@somaritane somaritane left a comment

Choose a reason for hiding this comment

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

lgtm!

Copy link
Contributor

@somaritane somaritane left a comment

Choose a reason for hiding this comment

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

lgtm

@ytsarev
Copy link
Member Author

ytsarev commented Dec 16, 2020

https://github.com/admiraltyio/admiralty/releases/tag/v0.13.2 ingress annotation trigger released so we can merge this doc

@ytsarev ytsarev merged commit 27049da into master Dec 16, 2020
@ytsarev ytsarev deleted the admiralty branch December 16, 2020 08:54
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.

5 participants