Skip to content
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.

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

rename sparseCheckoutDir from StarterProject #129

Merged
merged 2 commits into from
Oct 20, 2020

Conversation

kadel
Copy link
Member

@kadel kadel commented Sep 15, 2020

What does this PR do?

rename sparseCheckoutDir to subDir and use it only in StarterProject

What issues does this PR fix or reference?

#128

Is your PR tested? Consider putting some instruction how to test your changes

Docs PR

@kadel kadel changed the title Remove sparse checkout rename sparseCheckoutDir and remove it from Project source Sep 15, 2020
Copy link
Member

@sleshchenko sleshchenko left a comment

Choose a reason for hiding this comment

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

LGTM
@davidfestal please validate as well

Copy link
Collaborator

@davidfestal davidfestal left a comment

Choose a reason for hiding this comment

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

OK for me.

Side question: I'm still wondering whether we should keep both the git and github project source types (that until now have the same structure).
Do we want to keep both for 2.0.0 or only keep git for now, and add the github union value in the future if really necessary ?

cc @l0rd

@kadel
Copy link
Member Author

kadel commented Sep 16, 2020

Do we want to keep both for 2.0.0 or only keep git for now, and add the github union value in the future if really necessary ?

This also crossed my mind. I would actually prefer removing github for now. We can re-introduce it later once there are some extra github features that this can use.

@sunix
Copy link
Contributor

sunix commented Sep 16, 2020

Sorry just been throught this one. In Che, sparseCheckoutDir is doing a git sparsecheckout. There is no confusion.
What it also do is keep the git directory so user can still git commit and so on.
In real life projects (not just testing samples), user would usually commit/push changes of a project.

@kadel
Copy link
Member Author

kadel commented Sep 16, 2020

Sorry just been throught this one. In Che, sparseCheckoutDir is doing a git sparsecheckout. There is no confusion.
What it also do is keep the git directory so user can still git commit and so on.
In real life projects (not just testing samples), user would usually commit/push changes of a project.

In that case, we could keep sparseCheckoutDir in project (and do proper sparse checkout) and in StarterProject use subDir.

In this case, it might make sense to make sparseCheckoutDir a list to allow specify multiple dirs to checkout.

@sunix
Copy link
Contributor

sunix commented Sep 16, 2020

In that case, we could keep sparseCheckoutDir in project (and do proper sparse checkout) and in StarterProject use subDir.

In this case, it might make sense to make sparseCheckoutDir a list to allow specify multiple dirs to checkout.

+1

@kadel kadel changed the title rename sparseCheckoutDir and remove it from Project source rename sparseCheckoutDir from StarterProject Sep 18, 2020
@kadel kadel force-pushed the remove-sparse-checkout branch from c53dbc9 to 77fbf0c Compare September 18, 2020 12:42
@kadel
Copy link
Member Author

kadel commented Sep 18, 2020

updated,
@sunix @davidfestal please let me know if this works for you.

Copy link
Contributor

@amisevsk amisevsk left a comment

Choose a reason for hiding this comment

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

Looks fine in general but I'm still not clear on the distinction between sparseCheckoutDirs and subDir. At first glance, it looks like we have similar functionality with two different names.

Re-read the discussion; subPath = "grab only this directory" and sparseCheckout = git clone the whole project but do a sparse checkout. If so, that makes sense.

@sunix
Copy link
Contributor

sunix commented Sep 22, 2020

hey @kadel,
We had that use case: several projects in the same git repo. For instance 1 maven project in one sub path and 1 nodejs project in another sub path. For che, that was what one of the proposal: eclipse-che/che#15347
Let me know if this is the purpose of subDir. But if it is the case, it should be able to take multiple directories.

@davidfestal
Copy link
Collaborator

@kadel didn't you need that before the ODO release (since it would be a breaking change) ? Is there a reason it hasn't been merged ?

kadel added 2 commits October 8, 2020 15:56
this also changes SparseCheckoutDirs to list to allow specifing mulitple
directories
@kadel kadel force-pushed the remove-sparse-checkout branch from 77fbf0c to ed5f11b Compare October 8, 2020 14:07
@kadel
Copy link
Member Author

kadel commented Oct 8, 2020

@kadel didn't you need that before the ODO release (since it would be a breaking change) ? Is there a reason it hasn't been merged ?

We worked around this in odo by removing sparceChecoutDir from all devfiles used by odo.

@kadel
Copy link
Member Author

kadel commented Oct 20, 2020

Re-read the discussion; subPath = "grab only this directory" and sparseCheckout = git clone the whole project but do a sparse checkout. If so, that makes sense.

yes, but with one important distinction.

  • subPath is only for starterProject
  • sparseCheckout is only for "main" project

subPath is not there for project, because it doesn't make sense. You can't do what subPath is doing ("grab only this directory") and end up with valid git repo.
sparseCheckout is not there for starterProject because we don't need it it wouldn't make much sense to do sparseCheckout for starterProject. For starterProject we want to just grab a specific directory not to do sparse checkout.

@kadel kadel merged commit 77074ba into devfile:master Oct 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants