Add `ci --fetch-depth`, deprecate `ci --unshallow` #1233

deepyaman · 2022-10-19T15:09:37Z

Close #975

Development notes

I copied some stuff from https://github.com/actions/checkout/, including the description for the --fetch-depth option and the default values/error handling. One thing I didn't copy from there is the floor logic in https://raw.githubusercontent.com/actions/checkout/main/dist/index.js:

        result.fetchDepth = Math.floor(Number(core.getInput('fetch-depth') || '1'));

...but I could?

dacbd

@deepyaman looks good so far

src/cml.js

…ture/ci-fetch-depth

dacbd

@deepyaman sorry for the delay. I have done some testing and identified a few issues.

Additionally @iterative/cml I'm not sure what or if its because GitHub Actions way that the action/checkout clones repos but providing a non-0 value doesn't result in the expected behavior... see here: https://github.com/iterative/cml-playground/actions/runs/3315755706/jobs/5476740449#step:5:2, however, running this locally does work as expected.

src/cml.js

Co-authored-by: Daniel Barnes <[email protected]>

deepyaman · 2022-10-24T20:54:33Z

@deepyaman sorry for the delay. I have done some testing and identified a few issues.

No worries, and thanks for the comments! I've gone ahead and applied the requested changes.

bin/cml/repo/prepare.js

src/cml.js

casperdcl · 2022-10-24T23:12:57Z

src/cml.js

@@ -474,10 +475,13 @@ class CML {
    await exec(await driver.updateGitConfig({ userName, userEmail, remote }));
    if (unshallow) {
      if ((await exec('git rev-parse --is-shallow-repository')) === 'true') {
-        await exec('git fetch --unshallow');
+        return await exec('git fetch --unshallow');


isn't this better?

Suggested change

return await exec('git fetch --unshallow');

fetchDepth = 0;

Also do we need to add git rev-parse --is-shallow-repository below?

I don't think so, but I haven't played with this. does git rev-parse --is-shallow-repository return true if the depth was already say 3? we wouldn't want to circumvent being able to expand this out directly.

Needed to prevent errors; fixed as part of #1246

Co-authored-by: Casper da Costa-Luis <[email protected]>

dacbd

@deepyaman with deepyaman#1 we should be good to merge!

Assignment to constant variable

deepyaman · 2022-10-27T19:49:56Z

@deepyaman with deepyaman#1 we should be good to merge!

Merged it in, thanks!

FWIW when I looked through the code earlier to make a similar change (I think on other PRs), I think most of them just turned the whole block into let to handle cases that included non-constant variables, but I think this is better/safer?

dacbd

@deepyaman the first version probably didn't re-assign anything, If the linter sees the value is never update it changes it to a const.

stale

casperdcl

also follow-up https://github.com/iterative/cml.dev issue/PR?

casperdcl · 2022-10-29T00:20:22Z

src/cml.js

@@ -469,15 +469,19 @@ class CML {
      userName = GIT_USER_NAME,
      remote = GIT_REMOTE
    } = opts;
+    let { fetchDepth = 1 } = opts;


wait won't this now by default truncate non-shallow clones (as per SO#46004595)?

i.e. cml ci will now undo actions/checkout.with.fetch-depth: 0?

it does appear so. I'll do a follow up

Add ci --fetch-depth, deprecate ci --unshallow

cb072dc

deepyaman temporarily deployed to external October 19, 2022 15:09 Inactive

dacbd suggested changes Oct 19, 2022

View reviewed changes

src/cml.js Outdated Show resolved Hide resolved

Edit formatting of expected cml ci --help output

609a68e

deepyaman temporarily deployed to external October 19, 2022 17:53 Inactive

Remove isNaN check, already handled by Yargs lib

d34a56d

deepyaman temporarily deployed to external October 19, 2022 17:59 Inactive

Merge branch 'master' into feature/ci-fetch-depth

1263197

0x2b3bfa0 temporarily deployed to external October 19, 2022 18:35 Inactive

deepyaman added 2 commits October 19, 2022 18:44

Fix negative fetchDepths using coerce in Yargs

fc8a939

Merge remote-tracking branch 'origin/feature/ci-fetch-depth' into fea…

c485f75

…ture/ci-fetch-depth

deepyaman temporarily deployed to external October 19, 2022 22:47 Inactive

deepyaman marked this pull request as ready for review October 19, 2022 22:47

deepyaman requested a review from dacbd October 22, 2022 04:16

dacbd mentioned this pull request Oct 24, 2022

cml#1233 --ci-fetch depth iterative/cml-playground#136

Open

dacbd suggested changes Oct 24, 2022

View reviewed changes

src/cml.js Outdated Show resolved Hide resolved

src/cml.js Outdated Show resolved Hide resolved

Apply suggestions from code review

064516c

Co-authored-by: Daniel Barnes <[email protected]>

deepyaman had a problem deploying to external October 24, 2022 20:53 Failure

casperdcl added external-request You asked, we did good-first-issue Good for newcomers (good-first-issue) enhancement New feature or request cml-ci Subcommand labels Oct 24, 2022

casperdcl previously requested changes Oct 24, 2022

View reviewed changes

Reduce duplicate logic, and replace coerce with different condition

628bb96

Co-authored-by: Casper da Costa-Luis <[email protected]>

deepyaman had a problem deploying to external October 25, 2022 00:12 Failure

Merge branch 'master' into feature/ci-fetch-depth

0a4f720

deepyaman had a problem deploying to external October 26, 2022 12:33 Failure

Merge branch 'master' into feature/ci-fetch-depth

e37dedf

dacbd had a problem deploying to external October 27, 2022 18:17 Failure

Assignment to constant variable

e6482d7

dacbd suggested changes Oct 27, 2022

View reviewed changes

dacbd assigned deepyaman Oct 27, 2022

Merge pull request #1 from iterative/internal-deepyaman

4a9c47f

Assignment to constant variable

deepyaman temporarily deployed to external October 27, 2022 19:48 Inactive

dacbd self-requested a review October 27, 2022 21:03

dacbd approved these changes Oct 27, 2022

View reviewed changes

dacbd enabled auto-merge (squash) October 27, 2022 21:05

dacbd merged commit bb04076 into iterative:master Oct 27, 2022

deepyaman deleted the feature/ci-fetch-depth branch October 28, 2022 02:16

casperdcl reviewed Oct 29, 2022

View reviewed changes

dacbd mentioned this pull request Oct 29, 2022

Remove default value for --fetch-depth #1246

Merged

casperdcl mentioned this pull request Nov 1, 2022

ref/ci: --fetch-depth iterative/cml.dev#364

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `ci --fetch-depth`, deprecate `ci --unshallow` #1233

Add `ci --fetch-depth`, deprecate `ci --unshallow` #1233

deepyaman commented Oct 19, 2022 •

edited

Loading

dacbd left a comment

dacbd left a comment

deepyaman commented Oct 24, 2022

casperdcl Oct 24, 2022

dacbd Oct 25, 2022

casperdcl Oct 31, 2022 •

edited

Loading

dacbd left a comment

deepyaman commented Oct 27, 2022

dacbd left a comment

casperdcl left a comment

casperdcl Oct 29, 2022

dacbd Oct 29, 2022

dacbd Oct 29, 2022

Add ci --fetch-depth, deprecate ci --unshallow #1233

Add ci --fetch-depth, deprecate ci --unshallow #1233

Conversation

deepyaman commented Oct 19, 2022 • edited Loading

Development notes

dacbd left a comment

Choose a reason for hiding this comment

dacbd left a comment

Choose a reason for hiding this comment

deepyaman commented Oct 24, 2022

casperdcl Oct 24, 2022

Choose a reason for hiding this comment

dacbd Oct 25, 2022

Choose a reason for hiding this comment

casperdcl Oct 31, 2022 • edited Loading

Choose a reason for hiding this comment

dacbd left a comment

Choose a reason for hiding this comment

deepyaman commented Oct 27, 2022

dacbd left a comment

Choose a reason for hiding this comment

casperdcl left a comment

Choose a reason for hiding this comment

casperdcl Oct 29, 2022

Choose a reason for hiding this comment

dacbd Oct 29, 2022

Choose a reason for hiding this comment

dacbd Oct 29, 2022

Choose a reason for hiding this comment

Add `ci --fetch-depth`, deprecate `ci --unshallow` #1233

Add `ci --fetch-depth`, deprecate `ci --unshallow` #1233

deepyaman commented Oct 19, 2022 •

edited

Loading

casperdcl Oct 31, 2022 •

edited

Loading