-
Notifications
You must be signed in to change notification settings - Fork 126
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
SNOW-1553756 use forked version 99design/keyring
without kwallet/secretservice to avoid dbus issue
#1296
Closed
sfc-gh-dszmolka
wants to merge
4
commits into
master
from
SNOW-1553756-intermediate-workaround-use-forked-keyring
Closed
SNOW-1553756 use forked version 99design/keyring
without kwallet/secretservice to avoid dbus issue
#1296
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
737ca2a
SNOW-1553756 https://github.com/snowflakedb/gosnowflake/issues/1183 f…
sfc-gh-dszmolka 3c3d428
* fix replace
sfc-gh-dszmolka dc2ad03
* clean up dependencies (go mod tidy)
sfc-gh-dszmolka 0e5376a
Merge branch 'master' into SNOW-1553756-intermediate-workaround-use-f…
sfc-gh-dszmolka File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why using replace? It was required in hashicorp, because it was a transitive dependency for them (their direct dependency was gosnowflake). For us it is a direct dependency, so we can use it directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm did not consider it but you're right, lets fix that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as we discussed: since the fork declares itself as 99design/keyring, cannot import it directly due to
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we have Jeffail/keyring library security approval?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's exactly the same as 99designs/keyring , just two modules completely deleted from the original. No other changes.
If you suspect we'll still require security approval for using an already approved module with less functionality, Slack me please with the procedure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Risk Assessment PR created: 6390
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this not going to be a problem -
i've checked what differs in between those library versions @sfc-gh-pfus
99designs/keyring@master...Jeffail:keyring:master
and it says in the readme:
"Unfortunately, any modules that import your module will also need to add a replace directive of their own."
so not sure if this change won't be a BCR for GO driver users
cc @sfc-gh-pfus @sfc-gh-dszmolka