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

make secretRef field optional #865

Merged
merged 16 commits into from Jul 22, 2019
Merged

make secretRef field optional #865

merged 16 commits into from Jul 22, 2019

Conversation

mitchdraft
Copy link
Contributor

@mitchdraft mitchdraft commented Jul 19, 2019

BOT NOTES:
resolves #864

@solo-git-bot
Copy link

solo-git-bot bot commented Jul 19, 2019

Issues linked to changelog:
#864

@mitchdraft mitchdraft marked this pull request as ready for review July 20, 2019 05:34
Copy link
Member

@yuval-k yuval-k left a comment

Choose a reason for hiding this comment

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

some minor comments.
looks good overall but since i didn't review the previous PRs i think someone with more context should also review

@@ -141,6 +144,11 @@ func upstreamInstanceToEndpoint(writeNamespace string, upstream *v1.Upstream, in
Namespace: writeNamespace,
},
}
contextutils.LoggerFrom(ctx).Debugw("instance from upstream",
zap.Any("upstream", upstream),
Copy link
Member

Choose a reason for hiding this comment

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

nit: zap.Any("upstream", upstream) -> "upstream", upstream
same for below

Choose a reason for hiding this comment

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

Elsewhere we’ve been using zap fields, so I prefer this just for consistency

By("verifying instance is ready - if this failed, you may need to restart the EC2 instance")
// stitch the url together to avoid bot spam
ec2Port := 80
ec2Url := fmt.Sprintf("http://%v:%v/metrics", strings.Join([]string{"52", "91", "199", "115"}, "."), ec2Port)
Copy link
Member

Choose a reason for hiding this comment

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

what is this ip?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's a dummy ec2 instance for testing

@mitchdraft mitchdraft added the work in progress signals bulldozer to keep pr open (don't auto-merge) label Jul 22, 2019
@mitchdraft
Copy link
Contributor Author

wip: need to add region config for env-derived sessions

@mitchdraft mitchdraft removed the work in progress signals bulldozer to keep pr open (don't auto-merge) label Jul 22, 2019
@mitchdraft
Copy link
Contributor Author

(no longer a wip) - added region config and tests using local credentials @yuval-k @rickducott

@soloio-bulldozer soloio-bulldozer bot merged commit 8d76a03 into master Jul 22, 2019
@soloio-bulldozer soloio-bulldozer bot deleted the optional-ec2-secret branch July 22, 2019 18:23
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.

Let EC2 Upstream credentials come from the environment
3 participants