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

Support netrc-based credentials in buf-setup-action #94

Merged
merged 23 commits into from
Jan 12, 2023
Merged

Support netrc-based credentials in buf-setup-action #94

merged 23 commits into from
Jan 12, 2023

Conversation

tonyli233
Copy link
Contributor

Added two optional input "buf_user" and "buf_token".
If both inputs are supplied, perform logging into Buf registry with the supplied credentials.

@elliotmjackson elliotmjackson changed the title Resolve TCN-986: Support netrc-based credentials in buf-setup-action Support netrc-based credentials in buf-setup-action Jan 10, 2023
Copy link
Contributor

@elliotmjackson elliotmjackson left a comment

Choose a reason for hiding this comment

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

Also make changes to the readme here to supply new config items

action.yml Outdated Show resolved Hide resolved
action.yml Outdated Show resolved Hide resolved
src/run.ts Outdated Show resolved Hide resolved
src/run.ts Outdated
Comment on lines 95 to 102
if (bufUser !== "") {
core.info(
`buf_user is supplied, must also supply buf_token to log into Buf registry`
);
} else if (bufToken !== "") {
core.info(
`buf_token is supplied, must also supply buf_user to log into Buf registry`
);
Copy link
Member

Choose a reason for hiding this comment

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

Instead of a nested if... else if block, we could probably use early returns to handle the log statements for readability:

if (bufUser !== "" && bufToken !== "" {
    // the logic you already have, which is great
    return null;
}
if (bufUser !== "") {
    core.info(`buf_user is supplied, must also supply...`)
    return null;
}
if (bufToken !== "") {
    // same logic
    return null;
}
core.info(`buf_user and buf_token are not configured, not logging into Buf Schema Registry`)
return null;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for bringing this up! I have updated this in a recent commit.

src/run.ts Outdated Show resolved Hide resolved
src/run.ts Outdated Show resolved Hide resolved
@doriable doriable self-requested a review January 10, 2023 16:53
Copy link
Member

@doriable doriable left a comment

Choose a reason for hiding this comment

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

I don't know why it submitted as approve, I mean to comment! Sorry!

@CLAassistant
Copy link

CLAassistant commented Jan 10, 2023

CLA assistant check
All committers have signed the CLA.

@tonyli233
Copy link
Contributor Author

I have made the changes requested and update the readme.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/run.ts Show resolved Hide resolved
README.md Outdated
Comment on lines 90 to 103
#### Buf username and Buf API token

Optionally, you can supply both `buf_user` and `buf_api_token` input so that it will perform
[Buf Schema Registry][bsr] (BSR) authentication at the same time. This will allow you to access
the private remote packages in BSR. This only works when both `buf_user` and `buf_api_token` are
supplied at the same time:

```yaml
steps:
- uses: bufbuild/buf-setup-action@v1.11.0
with:
buf_user: ${{ secrets.buf_user }}
buf_api_token: ${{ secrets.buf_api_token }}
```
Copy link
Contributor

Choose a reason for hiding this comment

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

i believe this should live under "Other Configurations" just like "Buf token" does

Copy link
Contributor

@elliotmjackson elliotmjackson Jan 12, 2023

Choose a reason for hiding this comment

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

Suggested change
#### Buf username and Buf API token
Optionally, you can supply both `buf_user` and `buf_api_token` input so that it will perform
[Buf Schema Registry][bsr] (BSR) authentication at the same time. This will allow you to access
the private remote packages in BSR. This only works when both `buf_user` and `buf_api_token` are
supplied at the same time:
```yaml
steps:
- uses: bufbuild/buf-setup-action@v1.11.0
with:
buf_user: ${{ secrets.buf_user }}
buf_api_token: ${{ secrets.buf_api_token }}
```
#### Buf username and Buf API token
If you are using Private [Remote Packages](https://docs.buf.build/bsr/remote-packages/overview) you may need to authenticate the entire system to successfully communicate with the [Buf Schema Registry][bsr]. To achieve this, supply both `buf_user` and `buf_api_token`. This will add your auth credentials to the `.netrc` and allow you to access the BSR from anything in your `PATH`.
```yaml
steps:
- uses: bufbuild/buf-setup-action@v1.11.0
with:
buf_user: ${{ secrets.buf_user }}
buf_api_token: ${{ secrets.buf_api_token }}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think place it in the "Parameters" section better describes its usage. So I think this can stay here to avoid confusion with "Buf token"

Copy link
Contributor

@elliotmjackson elliotmjackson Jan 12, 2023

Choose a reason for hiding this comment

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

so the two methods to auth with buf are separated? buf_token is also a parameter. what does that mean for your reasoning?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

buf_token is a env var instead of an input to buf-setup-action. That's why I want to separate them by "Input" and "Other Configuration". I just changed the section name from "parameter" to "input".

tonyli233 and others added 2 commits January 12, 2023 11:40
Co-authored-by: Elliot Jackson <13633636+elliotmjackson@users.noreply.github.com>
Copy link
Member

@doriable doriable left a comment

Choose a reason for hiding this comment

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

Stamping for the code changes and to unblock :)

@elliotmjackson elliotmjackson merged commit 3aad079 into bufbuild:main Jan 12, 2023
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.

4 participants