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.

Sign up for GitHub

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

Jump to bottom

Expose connectionName of CloudSQLInstance in connection secret #159

Merged
merged 1 commit into from
Feb 3, 2020
Merged

Expose connectionName of CloudSQLInstance in connection secret #159

merged 1 commit into from
Feb 3, 2020

Conversation

muvaf
Copy link
Member

@muvaf muvaf commented Feb 3, 2020

Description of your changes

Expose connectionName in the connection secret to enable CloudSQL proxy connections.

Checklist

I have:

  • Run make reviewable to ensure this PR is ready for review.
  • Ensured this PR contains a neat, self documenting set of commits.
  • Updated any relevant documentation, examples, or release notes.
  • Updated the dependencies in app.yaml to include any new role permissions.
  • Updated the stack resources documentation to include any new managed resources or classes.

@muvaf
cloudsql: expose connectionName in the connection secret to enable Cl…
75cb45a 
…oudSQL proxy connections

Signed-off-by: Muvaffak Onus <onus.muvaffak@gmail.com>
@muvaf muvaf requested review from negz and hasheddan February 3, 2020 16:01
@muvaf muvaf mentioned this pull request Feb 3, 2020
Open
hasheddan
hasheddan approved these changes Feb 3, 2020
View reviewed changes
Copy link
Member

@hasheddan hasheddan left a comment

Choose a reason for hiding this comment

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

This looks reasonable to me! Thanks for addressing this quickly @muvaf :) Have you done any manual testing? Referencing #158 for reviewers who may not have context.

@upbound-bot
Copy link
Collaborator

60% (+0.01%) vs master 60%

@negz negz merged commit 39b9ff3 into crossplane-contrib:master Feb 3, 2020
@negz
Copy link
Member

negz commented Feb 3, 2020

Whoops, hit merge before I noticed @hasheddan's comment around testing this. @muvaf please do ack here that this works in practice. :)

@hasheddan hasheddan mentioned this pull request Feb 3, 2020
Closed
@muvaf
Copy link
Member Author

muvaf commented Feb 4, 2020

@negz @hasheddan just tested and it works.

@dsyer dsyer mentioned this pull request Feb 4, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hasheddan hasheddan hasheddan approved these changes

@negz negz Awaiting requested review from negz

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@muvaf @upbound-bot @negz @hasheddan