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

Bookinfo - Namespace onboarding #17

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Bookinfo - Namespace onboarding #17

wants to merge 4 commits into from

Conversation

jhveras-avesha
Copy link
Contributor

Modifies bookinfo configuration yaml files to support namespace onboarding

@jhveras-avesha jhveras-avesha requested a review from a user June 21, 2022 13:17
@jhveras-avesha jhveras-avesha requested a review from pnavali as a code owner June 21, 2022 13:17
@@ -1,7 +1,7 @@
##################################################################################
# Details ServiceExport
##################################################################################
apiVersion: networking.kubeslice.io/v1beta1
apiVersion: mesh.avesha.io/v1beta1
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good observation. I copied this config file from the latest version of the internal doc and didn't notice this change.

@@ -55,7 +55,7 @@ resource "aws_instance" "ubuntu-ec2" {
provisioner "remote-exec" {
inline = [
"cd /tmp/examples",
"git checkout ec2",
"git checkout bookinfo",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are we doing a git checkout of bookinfo (or ec2)? Presumably once this merges to master we'd use the code on the master branch (not on the bookinfo... or ec2... branch)? Should this just use the code from whatever branch it is on?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's because I need to test the changes before they go to master. If I would just checkout master branch I wouldn't be able to see the modifications to bookinfo. This is the same problem I had with devops-510: I needed to checkout that branch to really test the changes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see... because you're using ec2 to test the bookinfo changes and its doing its own clone you need it to grab this branch. Ok, please be sure to do another commit after this to remove the checkout of the bookinfo branch.

Copy link
Collaborator

@eric978 eric978 left a comment

Choose a reason for hiding this comment

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

After the fix from JD and the clarification on the ec2 branch, it LGTM.

@eric978 eric978 self-requested a review June 21, 2022 15:35
@eric978
Copy link
Collaborator

eric978 commented Jun 21, 2022

Actually, I don't think kind.sh should be added a namespace for bookinfo (a kind.sh user will end up with a bookinfo namespace that they don't use). IMO all the bookinfo stuff should be in the bookinfo app/dir. This can be an example of adding a namespace to an existing slice (or adding a new slice... either way).

@jhveras-avesha
Copy link
Contributor Author

@eric978 That's a good point. What about the slice.yaml file? Can I reapply that same same file and just add the info about the new namespace? Should I create a new slice just for bookinfo?

@eric978
Copy link
Collaborator

eric978 commented Jun 21, 2022 via email

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.

3 participants