[update.sh] Parsing token inside each upstream #16289

sazriel26 · 2023-12-04T16:16:16Z

Have you followed the guidelines in our Contributing document?
Have you checked to ensure there aren't other open Pull Requests for the same change?
Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your changes? Here's an example.
Have you successfully run brew style with your changes locally?
Have you successfully run brew typecheck with your changes locally?
Have you successfully run brew tests with your changes locally?

This pull request is not for formula but for update.sh script to allow local token parsing inside each upstream

Move global token HOMEBREW_GITHUB_API_TOKEN support inside each upstream repository to parse local token (if any) and setup it accordingly (local supersede global token - this can be reordered if needed)

MikeMcQuaid · 2023-12-04T18:26:59Z

@sazriel26 can you explain the motivation for this PR rather than what it is doing? Thanks!

sazriel26 · 2023-12-04T18:36:38Z

@MikeMcQuaid Well I started using a private repository for hosting some formulas with access through Git token (thus ran brew tap sazriel26/private https://sazriel26:<git_token>@github.com/....)

Such token is not a "global" but rather a local dedicated to this repository (git config is able to retrieve it and brew update can update the repository accordingly)

This is not a must have just proposing you this contribution for reviewing if it can suit

MikeMcQuaid · 2023-12-04T18:42:01Z

@sazriel26 Ok, I understand a bit more about what you're doing here but not the "why" behind the pull request. What did it do before that you didn't like and what does it do now instead?

sazriel26 · 2023-12-05T09:59:34Z

@MikeMcQuaid Sorry for the late reply, need to wait until next working day for getting access to test environment (and hopefully having a fresh mind)

About this pull request, it is aiming to help update command to check through Github API for private repository if continuing or not the process like for public ones.

To illustrate, when UPSTREAM_REPOSITORY_URL (from git config remote.origin.url) is including a token reference (https://user:[email protected]/...), then the test

      if [[ "${UPSTREAM_REPOSITORY_URL}" == "https://github.com/"* ]]

fails due to URL with user and token and skip this section of code
Then update continues to fetch the repository (see brew update --verbose)

Checking if we need to fetch /opt/homebrew/Library/Taps/sazriel26/homebrew-***...
...
Fetching /opt/homebrew/Library/Taps/sazriel26/homebrew-***..

With this "patch", if there is a token provided, it is parsed and can supersede the global one for using the API of Github to help deciding on going ahead or skipping the fetch - see

[[ -z "${HOMEBREW_UPDATE_FORCE}" ]] && [[ "${UPSTREAM_SHA_HTTP_CODE}" == "304" ]] && exit

Not a "big deal" but I wanted to share this "enhancement" for your attention.

Thanks for your attention and patience about this matter

MikeMcQuaid

I understand a bit more what you're asking for here, thanks

Library/Homebrew/cmd/update.sh

… HEAD See discussion in #16289

sazriel26 · 2023-12-06T14:04:43Z

Is it ok?

MikeMcQuaid

Thanks for the PR! Just to set expectations: I think this feature is desirable but this PR is currently quite a long way off being an implementation we can merge and support. I find it very hard to read and understand right now and the error-handling in update.sh needs to be absolutely rock-solid otherwise we run a risk of breaking Homebrew's own update mechanism.

Thanks again!

Library/Homebrew/cmd/update.sh

Library/Homebrew/utils/url.sh

better wording Co-authored-by: Mike McQuaid <[email protected]>

sazriel26 · 2023-12-07T08:18:17Z

First thank you @MikeMcQuaid for your kind support and availability about this PR.

According to #16289 (review), it looks hard to satisfy some prerequisites in contributing for a newcomer like me, isn't it?

What about to discard this PR which is not ready at this moment for merge?

Thanks again for your understanding and sorry for the annoyance (including time consuming)

MikeMcQuaid · 2023-12-07T20:30:40Z

First thank you @MikeMcQuaid for your kind support and availability about this PR.

You're welcome ❤️

According to #16289 (review), it looks hard to satisfy some prerequisites in contributing for a newcomer like me, isn't it?

I think we can get there together; I'm just giving you the heads up that it may take a while.

Some newcomers contributions are merged very quickly, others are not. It depends on the readability of the code, the invasiveness of the change and the potential negative effects of any issues.

What about to discard this PR which is not ready at this moment for merge?

Up to you, I'm happy to keep working here if you are!

Thanks again for your understanding and sorry for the annoyance (including time consuming)

Thank you and no apologies needed; not annoyed here at all 😁

sazriel26 · 2023-12-10T15:52:48Z

Thanks @MikeMcQuaid !

Let's give a try this week and state about if it can suit or not

See you then.

github-actions · 2024-01-01T00:07:39Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

sazriel26 · 2024-01-07T12:23:32Z

@MikeMcQuaid With some delay, and thinking of the shell used (bash), I have rewritten this pull request accordingly (with help of BASH_REMATCH array)

MikeMcQuaid

Thanks @sazriel26. This is looking a lot clearer now. I've suggested some tweaks (that may need fixed up with brew style) to ensure your new logic is a fallback rather than taking priority or encompassing the old logic.

Library/Homebrew/cmd/update.sh

carlocab · 2024-01-15T03:18:42Z

Library/Homebrew/cmd/update.sh


      # HOMEBREW_UPDATE_FORCE and HOMEBREW_UPDATE_AUTO aren't modified here so ignore subshell warning.
      # shellcheck disable=SC2030
      if [[ "${UPSTREAM_REPOSITORY_URL}" == "https://github.com/"* ]]
      then
        UPSTREAM_REPOSITORY="${UPSTREAM_REPOSITORY_URL#https://github.com/}"
        UPSTREAM_REPOSITORY="${UPSTREAM_REPOSITORY%.git}"
+      elif [[ "${UPSTREAM_REPOSITORY_URL}" =~ https://(([^:@]+)(:([^@]+))?@)?github.com/(.*)$ ]]


Suggested change

elif [[ "${UPSTREAM_REPOSITORY_URL}" =~ https://(([^:@]+)(:([^@]+))?@)?github.com/(.*)$ ]]

elif [[ "${DIR}" != "${HOMEBREW_REPOSITORY}" ]] && [[ "${UPSTREAM_REPOSITORY_URL}" =~ https://(([^:@]+)(:([^@]+))?@)?github.com/(.*)$ ]]

I think this is what we want to do here based on discussion with @MikeMcQuaid.

sazriel26 · 2024-01-22T08:01:59Z

Can it suit better now?

carlocab

I'm convinced that this code does what it's intended and is unlikely to break anything important.

However, is this the correct API for providing repository/tap-specific credentials?

IIUC, encoding access credentials in the URL is insecure and discouraged. If so, then, at minimum, we should:

at least redact the credentials from the URL we pass to curl; or,
maybe consider a different API for passing these credentials.

Bo98 · 2024-02-01T12:33:19Z

redact the credentials from the URL we pass to curl

We're not passing auth in the URL to curl. We're decoding it from an already-encoded git URL and passing it to curl via --header.

On the credential retrieval side, it isn't fully foolproof. You can supply git with a plain URL and retrieve credentials via the keychain (where available) or use .extraheader (https://github.com/Homebrew/actions/blob/d54a6744d5fcdff54b45a9659f3e17f769389952/setup-homebrew/main.sh#L129). Though anything is better than nothing.

MikeMcQuaid · 2024-02-01T13:40:38Z

IIUC, encoding access credentials in the URL is insecure and discouraged.

In my experience: it's also the only method that 100% works across curl and git invocations cross-platform without other tooling and configuration.

That's not to say "let's encourage it" but it is a completely reasonable thing to do particularly given:

We're not passing auth in the URL to curl. We're decoding it from an already-encoded git URL and passing it to curl via --header.

MikeMcQuaid

Thanks for your first PR to Homebrew, @sazriel26, you rock and I appreciate your patience with all the back-and-forth!

p-linnane · 2024-02-06T09:27:25Z

@sazriel26 Can you please rebase this so we can get it merged? Thanks!

sazriel26 · 2024-02-06T13:24:46Z

@sazriel26 Can you please rebase this so we can get it merged? Thanks!
@p-linnane Sure, can you hint me about 🙏?
I can try to resync my forked repository so it would be as close as possible as current state of reference repository
How can I do a git rebase 🤗 ?

Thanks in advance

p-linnane · 2024-02-06T14:31:48Z

You can check the Git manual for this.

sazriel26 · 2024-02-07T16:24:03Z

Oops ! I have broken this pull request, isn't it?

When I have tried to reproduce the https://git-scm.com/book/en/v2/Git-Branching-Rebasing:

I have renamed current "master" (from which I started the PR) to "parse-git-token" (the experiment)
I have cloned the upstream/master to "master"
When I ran the

git rebase master

it is saying "Current branch parse-git-token is up to date."

Shall I reopen a new pull request from the parse-git-token (old master from which I have opened this PR)?

Thanks in advance and sorry for the inconvenience

MikeMcQuaid · 2024-02-12T14:09:54Z

it is saying "Current branch parse-git-token is up to date."

You need to instead run git fetch --all; git rebase origin/master (assuming origin is Homebrew/brew and not your fork).

Shall I reopen a new pull request from the parse-git-token (old master from which I have opened this PR)?

Yes, please.

sazriel26 · 2024-02-13T08:41:33Z

@MikeMcQuaid Thanks for your answer

I have created a draft pull request #16649 as demanded

Can you check if it suits the requirements of rebase ?

Thanks in advance

sazriel26 added 2 commits December 4, 2023 18:04

[update.sh] Parse local token in git remote.origin.url config

c9f6f14

Merge branch 'token-support-in-update'

fc9b64a

Move global token HOMEBREW_GITHUB_API_TOKEN support inside each upstream repository to parse local token (if any) and setup it accordingly (local supersede global token - this can be reordered if needed)

MikeMcQuaid reviewed Dec 5, 2023

View reviewed changes

Library/Homebrew/cmd/update.sh Outdated Show resolved Hide resolved

Library/Homebrew/cmd/update.sh Outdated Show resolved Hide resolved

Library/Homebrew/cmd/update.sh Outdated Show resolved Hide resolved

sazriel26 added 6 commits December 6, 2023 14:20

Release Candidate 1

9102492

Merge branch 'Homebrew:master' into master

195a57b

Release Candidate 2

d1c2d5f

Merge branch 'Homebrew:master' into token-support-in-update

f4c25a1

Merge remote-tracking branch 'sazriel26/token-support-in-update' into…

234d054

… HEAD See discussion in #16289

Merge branch 'sazriel26-tmp' into HEAD

06e79a2

MikeMcQuaid reviewed Dec 6, 2023

View reviewed changes

Update Library/Homebrew/cmd/update.sh

e7c1575

better wording Co-authored-by: Mike McQuaid <[email protected]>

github-actions bot added the stale No recent activity label Jan 1, 2024

sazriel26 added 3 commits January 7, 2024 13:40

Merge remote-tracking branch 'upstream/master'

29ba761

New implementation through BASH regexes match

079e8a0

Fix missing ;

16fb088

github-actions bot removed the stale No recent activity label Jan 7, 2024

sazriel26 added 2 commits January 7, 2024 14:37

Fix brew style on update.sh

2f3af88

Fix brew style on update.sh (2)

ed1c0fd

MikeMcQuaid reviewed Jan 9, 2024

View reviewed changes

Library/Homebrew/cmd/update.sh Outdated Show resolved Hide resolved

Library/Homebrew/cmd/update.sh Outdated Show resolved Hide resolved

[Library/Homebrew/cmd/update.sh] Release Candidate for pull request

bced796

carlocab reviewed Jan 15, 2024

View reviewed changes

sazriel26 added 4 commits January 22, 2024 09:36

Merge branch 'Homebrew:master' into master

7b5cc6a

Merge branch 'Homebrew:master' into devel

618a94f

[update.sh] New test with protection of Homebrew directory

5908569

Merge remote-tracking branch 'sazriel26/devel' into HEAD

816f785

MikeMcQuaid requested review from Bo98, carlocab and cho-m and removed request for apainintheneck January 22, 2024 12:08

carlocab reviewed Feb 1, 2024

View reviewed changes

carlocab approved these changes Feb 2, 2024

View reviewed changes

MikeMcQuaid approved these changes Feb 2, 2024

View reviewed changes

sazriel26 added 2 commits February 7, 2024 09:59

Merge branch 'Homebrew:master' into master

10dfd3c

Merge branch 'Homebrew:master' into master

4b8a187

sazriel26 deleted the branch Homebrew:master February 7, 2024 13:16

sazriel26 closed this Feb 7, 2024

sazriel26 deleted the master branch February 7, 2024 13:16

sazriel26 restored the master branch February 7, 2024 13:16

sazriel26 deleted the master branch February 7, 2024 13:31

github-actions bot added the outdated PR was locked due to age label Mar 15, 2024

github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[update.sh] Parsing token inside each upstream #16289

[update.sh] Parsing token inside each upstream #16289

sazriel26 commented Dec 4, 2023

MikeMcQuaid commented Dec 4, 2023

sazriel26 commented Dec 4, 2023

MikeMcQuaid commented Dec 4, 2023

sazriel26 commented Dec 5, 2023

MikeMcQuaid left a comment

sazriel26 commented Dec 6, 2023

MikeMcQuaid left a comment

sazriel26 commented Dec 7, 2023

MikeMcQuaid commented Dec 7, 2023

sazriel26 commented Dec 10, 2023

github-actions bot commented Jan 1, 2024

sazriel26 commented Jan 7, 2024

MikeMcQuaid left a comment

carlocab Jan 15, 2024

sazriel26 commented Jan 22, 2024

carlocab left a comment

Bo98 commented Feb 1, 2024 •

edited

Loading

MikeMcQuaid commented Feb 1, 2024

MikeMcQuaid left a comment

p-linnane commented Feb 6, 2024

sazriel26 commented Feb 6, 2024

p-linnane commented Feb 6, 2024

sazriel26 commented Feb 7, 2024

MikeMcQuaid commented Feb 12, 2024

sazriel26 commented Feb 13, 2024

	elif [[ "${UPSTREAM_REPOSITORY_URL}" =~ https://(([^:@]+)(:([^@]+))?@)?github.com/(.*)$ ]]
	elif [[ "${DIR}" != "${HOMEBREW_REPOSITORY}" ]] && [[ "${UPSTREAM_REPOSITORY_URL}" =~ https://(([^:@]+)(:([^@]+))?@)?github.com/(.*)$ ]]

[update.sh] Parsing token inside each upstream #16289

[update.sh] Parsing token inside each upstream #16289

Conversation

sazriel26 commented Dec 4, 2023

MikeMcQuaid commented Dec 4, 2023

sazriel26 commented Dec 4, 2023

MikeMcQuaid commented Dec 4, 2023

sazriel26 commented Dec 5, 2023

MikeMcQuaid left a comment

Choose a reason for hiding this comment

sazriel26 commented Dec 6, 2023

MikeMcQuaid left a comment

Choose a reason for hiding this comment

sazriel26 commented Dec 7, 2023

MikeMcQuaid commented Dec 7, 2023

sazriel26 commented Dec 10, 2023

github-actions bot commented Jan 1, 2024

sazriel26 commented Jan 7, 2024

MikeMcQuaid left a comment

Choose a reason for hiding this comment

carlocab Jan 15, 2024

Choose a reason for hiding this comment

sazriel26 commented Jan 22, 2024

carlocab left a comment

Choose a reason for hiding this comment

Bo98 commented Feb 1, 2024 • edited Loading

MikeMcQuaid commented Feb 1, 2024

MikeMcQuaid left a comment

Choose a reason for hiding this comment

p-linnane commented Feb 6, 2024

sazriel26 commented Feb 6, 2024

p-linnane commented Feb 6, 2024

sazriel26 commented Feb 7, 2024

MikeMcQuaid commented Feb 12, 2024

sazriel26 commented Feb 13, 2024

Bo98 commented Feb 1, 2024 •

edited

Loading