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 support for Secret Manager usage in JdbcToBigQuery #1278

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

philippebailer
Copy link

No description provided.

Copy link
Contributor

@bvolpato bvolpato left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution, LGTM! Running the checks

@bvolpato
Copy link
Contributor

Can you please mvn spotless:apply and commit? Other than that, it's looking good, thanks!

@brianhuangee
Copy link

@bvolpato I pushed up Spotless changes

@philippebailer
Copy link
Author

@bvolpato any chance we could get another review?

@philippebailer philippebailer requested a review from bvolpato March 4, 2024 16:51
@philippebailer
Copy link
Author

@bharadwaj-aditya was looking at other PRs and saw you were active recently any chances you could review this and trigger the tests?

@bvolpato if your around as well just trying to get this merged if possibel

@philippebailer
Copy link
Author

philippebailer commented Mar 8, 2024

@shreyakhajanchi @bharadwaj-aditya @bvolpato

Just trying to get this reviewed I'm not sure what the correct way to get attention is so I've just been looking at recently reviewed PRs and pulling ppl sorry for being a bother but any guidance would be appreciated

@liferoad
Copy link
Contributor

liferoad commented Mar 8, 2024

Will merge this later after the tests are done.

@philippebailer
Copy link
Author

thank you! @liferoad

@liferoad
Copy link
Contributor

liferoad commented Mar 9, 2024

@philippebailer can you check the failed tests?

@philippebailer
Copy link
Author

Looking into it @liferoad, thanks for running the tests

@liferoad
Copy link
Contributor

Can we also update the doc with one example to show how to use this?

@philippebailer
Copy link
Author

Sure I can update the docs, do you mind pointing me to the docs that need updating? @liferoad

sorry we tried looking into the test failures last week and were pretty confused as to how they were happening and then things picked up at work and this got put on hold but I'll be circling back to this next week and hopefully will be able to resolve this.

I know that other templates that use the secrets manage have a selector so I am planning to follow those examples since I the current solution seems to have issues with the get and I'm just not familiar enough with all this to understand why.

@geoffreylemaire
Copy link

Any update ?
I am also very interested in this feature

@zachberger
Copy link
Member

Any update?

@damccorm
Copy link
Contributor

damccorm commented Oct 14, 2024

It looks like this PR had failing checks which blocked merging. Logs for those tests have since expired, so not sure if its a false positive or real problem. @philippebailer could you please pull in the latest changes from master onto your branch? That will cause the checks to be rerun and will make sure no weird conflicts have come in since this PR was opened

EDIT: I tried this in #1937 - TBD on results

@damccorm
Copy link
Contributor

I tried these changes out, and they don't work. If you try to stage this template, it errors with:

An exception occured while executing the Java class. Value only available at runtime, but accessed from a non-runtime context: RuntimeValueProvider{propertyName=connectionURL, default=null}

The underlying problem here is that we're trying to access the ValueProvider value as we construct the pipeline/template, but that value doesn't exist yet. The proper approach here would be to either:

  1. (preferred) Extend the KMSEncryptedNestedValueProvider to handle this functionality directly
  2. Implement a new value provider which handles this check as part of its functionality and shells out to KMSEncryptedNestedValueProvider

My only hesitation with (1) is that it impacts more templates than just this, so we need to make extra sure we're not doing something weirdly incompatible with other template options.

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

Successfully merging this pull request may close these issues.

7 participants