jx import creates git repos as public, they should be private by default instead

[rawlingsj]

Summary

We should assume repos should be private by default else code and secrets could be accidentally leaked.

Expected behaviour

When importing a project, create Git repo as PRIVATE
When creating a new cluster environment repos are PRIVATE

Actual behaviour

When importing a project, Git repo is PUBLIC and not unsecured.

[rawlingsj]

/area import
/priority critical-urgent

[hferentschik]

Are this not really two issues? One for jx import and the repo which is created there and one for j jx boot and the env repos created during this process?

[hferentschik]

That said, it seems creation of repositories goes via gits.PickNewGitRepository which takes a gits.GitRepositoryOptions which contains a flag to make the repo private. So maybe we can update all callers to this functions in one go?

[hferentschik]

There is potential usability issue. The free option for Organizations on GitHub does not allow private repositories, so when trying to for example import an application one gets:

? Which organisation do you want to use? hf-bee
Using Git provider github at https://github.com
? Using organisation: hf-bee
? Enter the new repository name:  foo
Creating repository hf-bee/foo
error: Failed to create repository hf-bee/foo due to: POST https://api.github.com/orgs/hf-bee/repos: 422 Visibility can't be private. Please upgrade your subscription to create a new private repository. []

If private repositories are supposed to be the default, how do we want to handle this? I wonder whether the import should really ask about an organization anyways, but rather for an owner of the new repository which might be a user or organization.

[Larzack]

Is it possible to catch this response ( 422 Visibility can't be private ) and then set up the repo to public.

But for payed accounts, it should be private by default.

[hferentschik]

Is it possible to catch this response ( 422 Visibility can't be private ) and then set up the repo to public.

There might be a way to check, not sure, but I think if private is the new default, creating public repos should not happen automatically. In this case we need imo some explicit user confirmation.

[pmuir]

jx boot and the env repos created during this process?
Also for jx install

There might be a way to check, not sure, but I think if private is the new default, creating public repos should not happen automatically. In this case we need imo some explicit user confirmation.

The idiomatic way in Go would be to check the error text.

I think a confirmation at this point would be reasonable.

[hferentschik]

The idiomatic way in Go would be to check the error text.

Not sure whether this would be idiomatic, but that's one option. The rub is that this error might differ by provider so it might be better to add some check/validation beforehand to check whether a user can create a given repository. So we could add something like AllowPrivateRepos to GitProvider.

The other issues is that once the error occurs, the directory is in a bad state. Some directories and commits got already created, so if when then tries to do something like jx import --git-private=false . the import will fail with an error that for example the charts directory already exists. So part of the error handling would be to reset the import directory into its initial state.

Overall we would:

• switch logic and flags to create repositories private per default
• handle errors when private repositories cannot be created, potentially even trying to figure out whether a user can create private repositories prior trying to create them
• make sure state is properly reset when error occurs

[hferentschik]

I also suggest to change:

Which organisation do you want to use to something like Who should be the owner of the created repository. I believe the use of organization is misleading at this point. It does not really have to be an organization. I believe jx boot already made a similar change where it does not ask for a "organization", but rather "owner" of the env repositories. WDYT?

[hferentschik]

Actually, I'd also like to switch the field GitPrivate to GitPublic. Given the null value of a bool, one can never really tell whether a value of false for GitPrivate is really explicitly false or false as a default. In this case we should error to the safer side.

I know we don't have #5222 in place, but the longer we wait the harder it might get. With the boot approach the CRD should just be overwritten anyways on an upgrade. cc @daveconde

[pmuir]

Not sure whether this would be idiomatic, but that's one option.

All I mean by that is it's the way I've seen e.g. the SDK handle this sort of thing.

But in this case it certainly seems like it won't be enough.

Which organisation do you want to use to something like Who should be the owner of the created repository.

Agreed.

Actually, I'd also like to switch the field GitPrivate to GitPublic. Given the null value of a bool, one can never really tell whether a value of false for GitPrivate is really explicitly false or false as a default. In this case we should error to the safer side.

One way around this is to write migration code for the GitPrivate field into the marshaller. You can use this to set the default of GitPublic to be the inverse of the current value of GitPrivate. I did something similar (handling the misnaming of the spec field) here

jx/pkg/apis/jenkins.io/v1/types_users.go

Line 74 in 09d481e

func (u *User) UnmarshalJSON(bs []byte) error {

- in your case it's way simpler as the value is plain bool.

[hferentschik]

One way around this is to write migration code for the GitPrivate field into the marshaller. You can use this to set the default of GitPublic to be the inverse of the current value of GitPrivate. I did something similar (handling the misnaming of the spec field) here

jx/pkg/apis/jenkins.io/v1/types_users.go

Line 74 in 09d481e

func (u *User) UnmarshalJSON(bs []byte) error {

- in your case it's way simpler as the value is plain bool.

Ohh, nice one. That would work.

[hferentschik]

Re-opening this issue due to #5513 and #5507.

commit 026e029 reverted the originally merged commits for this issue