Add '--branch' to command 'repository add' #1287

daisy-ycguo · 2022-09-28T02:03:02Z

Changes

🧹 Add support for named branches in func repository add command

codecov · 2022-09-28T08:26:35Z

Codecov Report

Merging #1287 (26ed3f7) into main (322acc7) will increase coverage by 0.46%.
The diff coverage is 55.88%.

@@            Coverage Diff             @@
##             main    #1287      +/-   ##
==========================================
+ Coverage   47.15%   47.61%   +0.46%     
==========================================
  Files          72       72              
  Lines       10229    10259      +30     
==========================================
+ Hits         4823     4885      +62     
+ Misses       4980     4951      -29     
+ Partials      426      423       -3

Impacted Files	Coverage Δ
repositories.go	`53.38% <40.00%> (ø)`
repository.go	`70.38% <50.00%> (-2.05%)`	⬇️
cmd/repository.go	`58.23% <85.71%> (+1.37%)`	⬆️
invoke.go	`47.82% <0.00%> (ø)`
function.go	`79.56% <0.00%> (ø)`
cmd/invoke.go	`49.38% <0.00%> (ø)`
config/config.go	`64.91% <0.00%> (ø)`
docker/creds/credentials.go	`71.71% <0.00%> (ø)`
... and 4 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

lkingland

Thanks for the PR! A couple of comments:

Could we implement internally using URL fragment identifiers (#<refspec>?
The --branch flag is good to include in the CLI for discoverability, but I wonder if it would be better implemented internally using a #<branch> suffix on the URL. This will clean up the API, make --verbose mode correct by default, allow added repository URLs to be canonical, and allow us to specify tags as well ([url]#v1.2.3) without altering the code in any way. I am interested to hear anyone else's opinion.
When listing repositories in verbose mode: func repositories list --verbose, it would be nice to also see the branch. As stated above this would just work if using a #<refspec> URL fragment identifier suffix.
The usage text of the command needs to be updated to include the new flag.

  {{.Name}} repo add <name> <url> [--branch] [-r|--repositories] [-c|--confirm] [-v|--verbose]

If we do go down the suggested route of supporting refspec as a URL fragment, that would be nice to be mentioned in the command's help text as well.

lkingland · 2022-10-03T11:15:59Z

cmd/repository.go

@@ -191,6 +191,7 @@ func NewRepositoryAddCmd(newClient ClientFactory) *cobra.Command {
 	}

 	cmd.Flags().BoolP("confirm", "c", cfg.Confirm, "Prompt to confirm all options interactively (Env: $FUNC_CONFIRM)")
+	cmd.Flags().StringP("branch", "", "main", "The repository branch to be added. Default is main branch")


The command help text automatically includes the default value, so there is no need for the second sentence here.

Also, I do not think we should include a default, because cloning a repository automatically clones the default branch, and not everyone uses main.

Fixed in the new version.

lkingland · 2022-10-03T11:17:33Z

cmd/repository.go

@@ -317,7 +318,7 @@ func runRepositoryList(_ *cobra.Command, args []string, newClient ClientFactory)
 }

 // Add
-func runRepositoryAdd(_ *cobra.Command, args []string, newClient ClientFactory) (err error) {
+func runRepositoryAdd(cmd *cobra.Command, args []string, newClient ClientFactory) (err error) {


The cmd argument is not used, so it can either stay _, or (perhaps better yet) change the method signature to completely remove the argument (which would require an update to lines 240-241)

Fixed in the new version.

lkingland · 2022-10-03T11:24:09Z

cmd/repository.go

@@ -394,7 +397,7 @@ func runRepositoryAdd(_ *cobra.Command, args []string, newClient ClientFactory)

 	// Add repository
 	var n string
-	if n, err = client.Repositories().Add(params.Name, params.URL); err != nil {
+	if n, err = client.Repositories().Add(params.Name, params.URL, params.Branch); err != nil {


I understand that, here in the CLI, it makes sense to also allow setting branch using a flag, so if we go down the route of using a # in the URL to specify branch in the core library API, then it could be accomplished here with something like:

if cfg.Branch != "" { params.URL = params.URL + "#" + cfg.Branch }

.. and from that point on the core library API would remain the same, and with a test or two to ensure it works, we would be supporting any refspec; not just branches.

lkingland · 2022-10-03T11:24:38Z

cmd/repository_test.go

@@ -47,6 +47,7 @@ func TestRepository_Add(t *testing.T) {

 	// add [flags] <old> <new>
 	add.SetArgs([]string{
+		"--branch=main",


The addition of this argument to the test does not actually test anything, because it is set to the same value as the flag default.

Fixed in the new version

lkingland · 2022-10-03T11:26:16Z

repositories.go

@@ -166,14 +166,14 @@ func (r *Repositories) Get(name string) (repo Repository, err error) {
 // Add a repository of the given name from the URI.  Name, if not provided,
 // defaults to the repo name (sans optional .git suffix). Returns the final
 // name as added.
-func (r *Repositories) Add(name, uri string) (string, error) {
+func (r *Repositories) Add(name, uri, branch string) (string, error) {


As explained above, we may want to consider leaving this API the same, and expecting branch to be provided as part of the optional uri.

lkingland · 2022-10-03T11:45:56Z

repository.go

@@ -125,12 +127,13 @@ type templateConfig = funcDefaults
 //   the given URI.
 // URI (optional), the path either locally or remote from which to load
 //    the repository files.  If not provided, the internal default is assumed.
-func NewRepository(name, uri string) (r Repository, err error) {
+func NewRepository(name, uri, branch string) (r Repository, err error) {


Please update the doc string above this exported function with the new optional argument.

Thanks. fixed.

lance

Thanks for the PR. Looks good but I think I tend to agree with @lkingland regarding the use of an optional #<branchname> on the end of the URI. If that's the case, then you can ignore my two comments. :)

lance · 2022-09-30T20:17:16Z

cmd/repository.go

@@ -191,6 +191,7 @@ func NewRepositoryAddCmd(newClient ClientFactory) *cobra.Command {
 	}

 	cmd.Flags().BoolP("confirm", "c", cfg.Confirm, "Prompt to confirm all options interactively (Env: $FUNC_CONFIRM)")
+	cmd.Flags().StringP("branch", "", "main", "The repository branch to be added. Default is main branch")


Probably good to have $FUNC_BRANCH in the environment as well, adding it to the bindEnv() call on line 185.

Thanks. Fixed in the new version.

lance · 2022-09-30T20:56:06Z

cmd/repository.go

 	}{}
 	if len(args) > 0 {
 		params.Name = args[0]
 	}
 	if len(args) > 1 {
 		params.URL = args[1]
 	}
+	params.Branch, _ = cmd.Flags().GetString("branch")


Nit. Might be cleaner as

Suggested change

params.Branch, _ = cmd.Flags().GetString("branch")

params.Branch = viper.GetString("branch")

But also below... can you add a survey question for branch here?
https://github.com/knative-sandbox/kn-plugin-func/blob/abc4de5b8f064ec318ff2a4fba27163d34a64aa3/cmd/repository.go#L367-L384

daisy-ycguo · 2022-10-10T09:49:40Z

Thanks for the review. Just back from national holidays. working on a new version of PR now.

knative-prow · 2022-10-13T10:52:32Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: daisy-ycguo
Once this PR has been reviewed and has the lgtm label, please assign nainaz for approval by writing /assign @nainaz in a comment. For more information see:The Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

codecov · 2022-10-13T10:58:38Z

Codecov Report

Base: 47.20% // Head: 49.50% // Increases project coverage by +2.29% 🎉

Coverage data is based on head (f3045f7) compared to base (3f5933e).
Patch coverage: 58.33% of modified lines in pull request are covered.

❗ Current head f3045f7 differs from pull request most recent head 3b3c220. Consider uploading reports for the commit 3b3c220 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1287      +/-   ##
==========================================
+ Coverage   47.20%   49.50%   +2.29%     
==========================================
  Files          74       77       +3     
  Lines       10409    10571     +162     
==========================================
+ Hits         4914     5233     +319     
+ Misses       5069     4895     -174     
- Partials      426      443      +17

Flag	Coverage Δ
integration-tests	`44.05% <52.77%> (?)`
unit-tests-macos-latest	`46.18% <58.33%> (?)`
unit-tests-ubuntu-latest	`47.64% <58.33%> (?)`
unit-tests-windows-latest	`46.20% <58.33%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
repositories.go	`53.38% <40.00%> (ø)`
repository.go	`70.66% <54.16%> (-2.04%)`	⬇️
cmd/repository.go	`58.23% <85.71%> (+1.37%)`	⬆️
function.go	`79.48% <0.00%> (-0.08%)`	⬇️
docker/docker_client_nonlinux.go	`0.00% <0.00%> (ø)`
openshift/openshift.go	`5.76% <0.00%> (ø)`
openshift/metadata.go	`0.00% <0.00%> (ø)`
client.go	`65.18% <0.00%> (+0.60%)`	⬆️
s2i/builder.go	`62.40% <0.00%> (+1.19%)`	⬆️
... and 9 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

lance · 2022-10-31T14:01:38Z

Hi @daisy-ycguo just checking in to see if you've had a chance to look into these test failures.

knative-prow-robot · 2022-11-05T07:12:11Z

@daisy-ycguo: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

lance · 2023-01-10T14:46:48Z

Hi @daisy-ycguo just checking in here - any progress?

matejvasek · 2023-01-30T13:16:45Z

@daisy-ycguo gentle ping

knative-prow bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 28, 2022

daisy-ycguo changed the title ~~Add option 'branch' to command 'repository add'~~ [WIP] Add option 'branch' to command 'repository add' Sep 28, 2022

knative-prow bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 28, 2022

daisy-ycguo force-pushed the branch branch 3 times, most recently from 1bd7385 to bf9d0e4 Compare September 28, 2022 08:21

daisy-ycguo changed the title ~~[WIP] Add option 'branch' to command 'repository add'~~ Add option 'branch' to command 'repository add' Sep 28, 2022

knative-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 28, 2022

daisy-ycguo force-pushed the branch branch from bf9d0e4 to 26ed3f7 Compare September 28, 2022 08:48

daisy-ycguo changed the title ~~Add option 'branch' to command 'repository add'~~ Add '--branch' to command 'repository add' Sep 29, 2022

lkingland reviewed Oct 3, 2022

View reviewed changes

lance reviewed Oct 3, 2022

View reviewed changes

lance modified the milestones: 1.8.0 Release, 1.9.0 Release Oct 4, 2022

daisy-ycguo force-pushed the branch branch from 26ed3f7 to f3045f7 Compare October 13, 2022 10:52

add option 'branch' to command 'repository add'

3b3c220

daisy-ycguo force-pushed the branch branch from f3045f7 to 3b3c220 Compare October 13, 2022 10:58

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 5, 2022

lance modified the milestones: 1.9.0 Release, Release 1.10 Feb 14, 2023

lance mentioned this pull request Feb 15, 2023

feat: adds refspec support for func repo add #1558

Merged

knative-prow bot closed this in #1558 Feb 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add '--branch' to command 'repository add' #1287

Add '--branch' to command 'repository add' #1287

daisy-ycguo commented Sep 28, 2022 •

edited

Loading

codecov bot commented Sep 28, 2022 •

edited

Loading

lkingland left a comment •

edited

Loading

lkingland Oct 3, 2022

lkingland Oct 3, 2022 •

edited

Loading

daisy-ycguo Oct 11, 2022

lkingland Oct 3, 2022

daisy-ycguo Oct 13, 2022

lkingland Oct 3, 2022 •

edited

Loading

lkingland Oct 3, 2022

daisy-ycguo Oct 13, 2022

lkingland Oct 3, 2022

lkingland Oct 3, 2022

daisy-ycguo Oct 13, 2022

lance left a comment

lance Sep 30, 2022

daisy-ycguo Oct 13, 2022

lance Sep 30, 2022

daisy-ycguo commented Oct 10, 2022

knative-prow bot commented Oct 13, 2022

codecov bot commented Oct 13, 2022 •

edited

Loading

lance commented Oct 31, 2022

knative-prow-robot commented Nov 5, 2022

lance commented Jan 10, 2023

matejvasek commented Jan 30, 2023

	params.Branch, _ = cmd.Flags().GetString("branch")
	params.Branch = viper.GetString("branch")

Add '--branch' to command 'repository add' #1287

Add '--branch' to command 'repository add' #1287

Conversation

daisy-ycguo commented Sep 28, 2022 • edited Loading

Changes

codecov bot commented Sep 28, 2022 • edited Loading

Codecov Report

lkingland left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lkingland Oct 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lkingland Oct 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lance left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daisy-ycguo commented Oct 10, 2022

knative-prow bot commented Oct 13, 2022

codecov bot commented Oct 13, 2022 • edited Loading

Codecov Report

lance commented Oct 31, 2022

knative-prow-robot commented Nov 5, 2022

lance commented Jan 10, 2023

matejvasek commented Jan 30, 2023

daisy-ycguo commented Sep 28, 2022 •

edited

Loading

codecov bot commented Sep 28, 2022 •

edited

Loading

lkingland left a comment •

edited

Loading

lkingland Oct 3, 2022 •

edited

Loading

lkingland Oct 3, 2022 •

edited

Loading

codecov bot commented Oct 13, 2022 •

edited

Loading