Skip to content

Simplify credential reading, support plain-text netrc only. #179

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
Merged
ericdallo merged 1 commit into from
Oct 29, 2025
Merged

Simplify credential reading, support plain-text netrc only. #179

ericdallo merged 1 commit into from
Oct 29, 2025

Conversation

@sg-qwt
Copy link
Contributor

@sg-qwt sg-qwt commented Oct 27, 2025 •
edited
Loading

This is a follow up on #146 to simplify our credential reading logic.

  1. drop authinfo and gpg decryption logic out of scope
  2. added a --netrc-file option for flexible use-cases

Basically same as curl's implementation.

  • I added a entry in changelog under unreleased section.

ericdallo
ericdallo reviewed Oct 27, 2025
View reviewed changes
Copy link
Member

@ericdallo ericdallo left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thank you, looks good

I was meaning to ask to add tests to the --netrc-file, but then I started to wonder: what is the best here: a --netrc-file CLI option or a ECA config :netrc-file.

FYI @r0man

@sg-qwt
Copy link
Contributor Author

sg-qwt commented Oct 28, 2025 •
edited
Loading

I was meaning to ask to add tests to the --netrc-file, but then I started to wonder: what is the best here: a --netrc-file CLI option or a ECA config :netrc-file.

I think --netrc-file in cli would be most convenient in case people would like to improvise in shell (pipe things around etc..).

A ECA config :netrc-file shall also work, and makes the cli a little cleaner. Even a env var like ECA_NETRC_FILE would be fine.

Personally I don't have a preference. Let me know what you think.

Also what's the best way to test this? An integration test?

@ericdallo
Copy link
Member

ericdallo commented Oct 28, 2025

I prefer to have netrc-file as a config for now, probably will fix all cases, people can always have local configs for that.

having config, you will only need to test the netrc feature itself reading from config as unit tests

@sg-qwt sg-qwt marked this pull request as draft October 28, 2025 16:16
@sg-qwt
Simplify credential reading, support plain-text netrc only.
1408971 
drop authinfo and gpg decryption logic
added a netrcFile in eca config for flexible use-cases
@sg-qwt sg-qwt marked this pull request as ready for review October 29, 2025 07:17
@sg-qwt
Copy link
Contributor Author

sg-qwt commented Oct 29, 2025

@ericdallo pls check the new imp with netrcFile ECA config

ericdallo
ericdallo approved these changes Oct 29, 2025
View reviewed changes
Copy link
Member

@ericdallo ericdallo left a comment

Choose a reason for hiding this comment

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

Looks great, thank you!

@ericdallo ericdallo merged commit b09c6ed into editor-code-assistant:master Oct 29, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ericdallo ericdallo ericdallo approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@sg-qwt @ericdallo