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

[issue 679][oauth2] Fix macos compiler warnings, remove store impleme… #737

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

llqgit
Copy link

@llqgit llqgit commented Feb 25, 2022

…nt keyring which not used at all

Master Issue: #719

Motivation

Fix MacOS version warning from the unused OAuth2 store implement named keyring.go

Modifications

To fix the warning that on MacOS once for all by remove the keyring.go which no references from other places, rather than upgrade the keyring package.

Verifying this change

  • Make sure that the change passes the CI checks.

This change is already covered by existing tests, such as (please describe tests).

Does this pull request potentially affect one of the following parts:

  • Dependencies (does it add or upgrade a dependency): (yes)
    • removed github.com/99designs/go-keychain

Documentation

  • Does this pull request introduce a new feature? (no)

@llqgit
Copy link
Author

llqgit commented Mar 25, 2022

Any change to confirm PR ?

@shoothzj
Copy link
Member

@wolfstudy can you confirm this PR? thanks

@wolfstudy wolfstudy requested a review from zymap April 13, 2022 03:56
@wolfstudy wolfstudy added this to the v0.9.0 milestone Apr 13, 2022
@wolfstudy
Copy link
Member

cc @zymap Please take a look thanks

Copy link
Member

@zymap zymap left a comment

Choose a reason for hiding this comment

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

It's a persistent implementation, we shouldn't remove it. It's an option of the store implementation, user can choose by themselves.

@llqgit
Copy link
Author

llqgit commented Apr 16, 2022

@zymap I got it and understand your point.
And I still have a question about it.

If it should be kept for the users which choose this implementation, now what's your opinion about the users not use this extra implementation and still shown the warning below?

It is really annoying when you develop and run the program in every time.

image

@zymap
Copy link
Member

zymap commented Apr 18, 2022

I saw the author has fixed this issue(keybase/go-keychain#76). Could you please try to upgrade the dependency to check if it works?

@llqgit
Copy link
Author

llqgit commented Apr 18, 2022

@zymap I just run:

go mod init my_project
go mod tidy

This is the official way to init, it should be all right.
And the dependences like this:

image

image

If I run my program, the errors shown like this:
image

You said the new version of github.com/keybase/go-keychain fix the problem.
So, if I just upgrade this package by go get github.com/keybase/go-keychain CMD.

The version would be:
image

It should be good. But I got some new errors which looks worse:

image

By the way. My mac OS version

image

From my perspective, the warning problem was not fixed yet or not effective.

@ciiiii
Copy link

ciiiii commented Apr 21, 2022

@llqgit I think they remove the support to avoid the warning. keybase/go-keychain#76

@freeznet freeznet modified the milestones: v0.9.0, v0.10.0 Jul 4, 2022
@RobertIndie RobertIndie modified the milestones: v0.10.0, v0.11.0 Mar 27, 2023
@RobertIndie RobertIndie modified the milestones: v0.11.0, v0.12.0 Jul 4, 2023
@RobertIndie RobertIndie modified the milestones: v0.12.0, v0.13.0 Jan 10, 2024
@RobertIndie RobertIndie modified the milestones: v0.13.0, v0.14.0 Jul 15, 2024
@RobertIndie RobertIndie modified the milestones: v0.14.0, v0.15.0 Oct 8, 2024
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.

7 participants