fix(github): allow only pull permission for print command #262

jetersen · 2022-07-18T21:28:09Z

Without configuring platform run options. Not all repos are downloaded due to conditions.

What does this change

Instead of:
DEBU[0000] Skipping repository since the token does not have push permissions and the run will not fork repo=jenkinsci/opslevel-plugin
INFO[0000] Running on 0 repositories

You get:
INFO[0000] Running on 1 repositories
INFO[0000] Cloning and running script repo=jenkinsci/opslevel-plugin

Since jenkinsci/opslevel-plugin is a fork I have no push permissions and g.Fork always being false because configureRunPlatform is never run.

Notes for the reviewer

Put any questions or notes for the reviewer here.

Checklist

Made sure the PR follows the CONTRIBUTING.md guidelines
Tests if something new is added

lindell · 2022-07-19T18:50:12Z

Thanks for the PR. The bug is definitely real and needs to be addressed. The print command should only require pull permissions.

Unfortunately, this does more than that. It does add the fork option (as the title suggests), but that option is intentionally kept out of the print command since there are no forks to be considered when running print. The fix should be to ensure that only pull permissions are required and nothing else.

jetersen · 2022-07-19T18:51:47Z

Well even though there is no fork. It filters out the repository. So I wonder what the conditional logic should say.

lindell · 2022-07-19T18:58:15Z

It would have to be indicated to each VersionController that push permission is not needed.

Adding something like "readOnly" and change this

multi-gitter/internal/scm/github/github.go

Line 146 in 60bbbdf

case !g.Fork && !permissions["push"]:

to

case !g.Fork && !g.ReadOnly && !permissions["push"]:

jetersen · 2022-07-19T19:02:10Z

I'll see if I can get around to it in the weekend to add readonly.

jetersen · 2022-07-29T10:00:06Z

Hopefully this weekend!

jetersen · 2022-07-30T12:23:06Z

@lindell I added flag that is set programmatically in the printCmd() method. To not change the signature of other methods like getVersionController and createGithubClient

jetersen · 2022-07-30T12:28:39Z

cmd/platform.go

+	// This is only used by PrintCmd to mark readOnly mode for version control platform
+	flags.Bool("readOnly", false, "If set, This is running in readonly will be read-only.")
+	_ = flags.MarkHidden("readOnly")


This is added inside configurePlatform which is run for all cmds.
Marking it hidden as this is set programmatically by the print command.

jetersen · 2022-07-30T12:29:28Z

cmd/cmd-print.go

@@ -48,6 +48,8 @@ func print(cmd *cobra.Command, args []string) error {
 	strOutput, _ := flag.GetString("output")
 	strErrOutput, _ := flag.GetString("error-output")

+	_ = flag.Set("readOnly", "true")


ignoring the error here because we know readOnly flag is available.

jetersen · 2022-07-30T12:30:41Z

internal/scm/github/github.go

@@ -143,7 +148,7 @@ func (g *Github) GetRepositories(ctx context.Context) ([]scm.Repository, error)
 		case !permissions["pull"]:
 			log.Debug("Skipping repository since the token does not have pull permissions")
 			continue
-		case !g.Fork && !permissions["push"]:
+		case !g.Fork && !g.ReadOnly && !permissions["push"]:


testing against

repo: - jenkinsci/jenkins-infra-test-plugin # a non forked repo - jenkinsci/opslevel-plugin # a forked repo

lindell · 2022-08-05T18:03:02Z

Thanks for the PR @jetersen.

@lindell I added flag that is set programmatically in the printCmd() method. To not change the signature of other methods like getVersionController and createGithubClient

I don't think we are able to do this in a good way, since the flags are used to automatically generate documentation and the help text. We do also expose something, that if set by the user, would make multi-gitter not work properly and create confusion. Changing the signature of getVersionController and createGithubClient has to be done in this case, and that is ok 😄

This logic should also extend to other SCMs, if you are able to add to them as well, that would be appreciated. I can make sure to test it.

jetersen · 2022-08-06T06:12:10Z

I did not see the same permission check in the other SCM

lindell · 2022-08-06T11:21:52Z

I did not see the same permission check in the other SCM

Indeed that is the case. Nvm that comment

jetersen · 2022-08-06T11:52:47Z

@lindell I believe mark hidden does hide it from help text and usage: See docs: https://github.com/spf13/pflag/blob/85dd5c8bc61cfa382fecd072378089d4e856579d/README.md#hidden-flags

So I see no reason to change this further.
As the code achieves the desired effect. A hidden internal flag we can use signal other code that we are in print mode.

I can change the name of the flag to something like secretReadOnly or printReadOnly

jetersen · 2022-08-06T11:59:00Z

Also sees relatively frequent usage:
https://cs.github.com/?q=MarkHidden%20language%3AGo&scopeName=All%20repos&scope=
https://github.com/search?q=MarkHidden+language%3Ago&type=code

lindell · 2022-08-06T12:04:57Z

Seems that it hides it from the help text, but we have some other parts that generate docs that do not take it into consideration (yaml documentation in readme). That could also be fixed to check for hidden flags.

But depending on hidden flags to pass data around (that should not be set by setting the flag) is unfortunately not something I can accept. It does indeed work, but so does a global variable, or environment variables etc.

lindell · 2022-08-06T12:06:04Z

Also sees relatively frequent usage: https://cs.github.com/?q=MarkHidden%20language%3AGo&scopeName=All%20repos&scope= https://github.com/search?q=MarkHidden+language%3Ago&type=code

MarkHidden should have a lot of legit usecases, but I can't find any place in the top search results where it is simply used as a way to create a hidden flags where it is only set programmatically.

jetersen · 2022-08-06T12:11:19Z

https://github.com/timothysc/clusteradm/blob/01435a84f9b5dee6717df68300458112437c32f9/main.go#L28-L34 ? 🤔

jetersen · 2022-08-06T12:22:08Z

@lindell fixed. Thanks to goland refactoring it was a 30 seconds change. 👏

If we add more argument passing we should switch to passing a struct.

lindell · 2022-08-06T12:44:11Z

Thanks 😄

If we add more argument passing we should switch to passing a struct.

Totally agree 👍

github-actions · 2022-08-06T12:53:14Z

Included in release v0.42.1 🎉

jetersen · 2022-08-06T13:54:22Z

@lindell Thanks for accepting my pull request! 👏🎉

jetersen changed the title ~~fix print being unaware of forks~~ fix print being unaware of fork option, which prevents cloning repositories without push permissions Jul 18, 2022

jetersen force-pushed the patch-1 branch from 14e72bc to 3eb7c08 Compare July 30, 2022 12:22

jetersen changed the title ~~fix print being unaware of fork option, which prevents cloning repositories without push permissions~~ fix GitHub permission check for print cmd Jul 30, 2022

jetersen force-pushed the patch-1 branch from 3eb7c08 to 0b26b36 Compare July 30, 2022 12:26

jetersen commented Jul 30, 2022

View reviewed changes

jetersen force-pushed the patch-1 branch from 0b26b36 to f1b6402 Compare July 30, 2022 12:33

add readOnly check

6480066

jetersen force-pushed the patch-1 branch from f1b6402 to 6480066 Compare July 30, 2022 12:36

pass down readOnly instead of using flag

7b84d8c

lindell changed the title ~~fix GitHub permission check for print cmd~~ fix(github): permission check for print command Aug 6, 2022

lindell changed the title ~~fix(github): permission check for print command~~ fix(github): allow only pull permission for print command Aug 6, 2022

lindell merged commit 582c706 into lindell:master Aug 6, 2022

jetersen deleted the patch-1 branch August 6, 2022 13:53

This was referenced Jan 6, 2024

chore(master): release 1.0.0 #448

Closed

chore(master): release 1.0.0 #449

Closed

github-actions bot mentioned this pull request Jul 2, 2024

chore(master): release 0.52.1 #491

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(github): allow only pull permission for print command #262

fix(github): allow only pull permission for print command #262

jetersen commented Jul 18, 2022 •

edited

Loading

lindell commented Jul 19, 2022

jetersen commented Jul 19, 2022

lindell commented Jul 19, 2022

jetersen commented Jul 19, 2022

jetersen commented Jul 29, 2022

jetersen commented Jul 30, 2022 •

edited

Loading

jetersen Jul 30, 2022

jetersen Jul 30, 2022

jetersen Jul 30, 2022

lindell commented Aug 5, 2022

jetersen commented Aug 6, 2022

lindell commented Aug 6, 2022

jetersen commented Aug 6, 2022 •

edited

Loading

jetersen commented Aug 6, 2022

lindell commented Aug 6, 2022

lindell commented Aug 6, 2022

jetersen commented Aug 6, 2022

jetersen commented Aug 6, 2022 •

edited

Loading

lindell commented Aug 6, 2022

github-actions bot commented Aug 6, 2022

jetersen commented Aug 6, 2022

fix(github): allow only pull permission for print command #262

fix(github): allow only pull permission for print command #262

Conversation

jetersen commented Jul 18, 2022 • edited Loading

What does this change

Notes for the reviewer

Checklist

lindell commented Jul 19, 2022

jetersen commented Jul 19, 2022

lindell commented Jul 19, 2022

jetersen commented Jul 19, 2022

jetersen commented Jul 29, 2022

jetersen commented Jul 30, 2022 • edited Loading

jetersen Jul 30, 2022

Choose a reason for hiding this comment

jetersen Jul 30, 2022

Choose a reason for hiding this comment

jetersen Jul 30, 2022

Choose a reason for hiding this comment

lindell commented Aug 5, 2022

jetersen commented Aug 6, 2022

lindell commented Aug 6, 2022

jetersen commented Aug 6, 2022 • edited Loading

jetersen commented Aug 6, 2022

lindell commented Aug 6, 2022

lindell commented Aug 6, 2022

jetersen commented Aug 6, 2022

jetersen commented Aug 6, 2022 • edited Loading

lindell commented Aug 6, 2022

github-actions bot commented Aug 6, 2022

jetersen commented Aug 6, 2022

jetersen commented Jul 18, 2022 •

edited

Loading

jetersen commented Jul 30, 2022 •

edited

Loading

jetersen commented Aug 6, 2022 •

edited

Loading

jetersen commented Aug 6, 2022 •

edited

Loading