Skip to content

Node.js SDK - CLI Downloader#4417

Closed
myty wants to merge 12 commits into
dagger:mainfrom
myty:feature/sdk-nodejs-windows-cli-downloader
Closed

Node.js SDK - CLI Downloader#4417
myty wants to merge 12 commits into
dagger:mainfrom
myty:feature/sdk-nodejs-windows-cli-downloader

Conversation

@myty
Copy link
Copy Markdown
Contributor

@myty myty commented Jan 20, 2023

Additional changes identified from #4390

  • separate cli-downloader functionality from Bin class and move it into its own class, CliDownloader
  • updates tests

myty added 4 commits January 16, 2023 17:30
@myty
fix: make sdk/nodejs windows friendly
7212af9 
- separate cli-downloader functionality from Bin class and move it into it's own class
- updates tests

cleanup connect test

cleanup connect test

Signed-off-by: Michael Tyson <myty@users.noreply.github.com>
@myty
Merge branch 'main' into bugfix/cli-downloader-node-sdk-windows
881a3af
@myty
Merge branch 'main' into feature/sdk-nodejs-windows-cli-downloader
22fa4c3 
Signed-off-by: Michael Tyson <myty@users.noreply.github.com>
@myty
add getCliBinPath method to connect
f3923aa 
Signed-off-by: Michael Tyson <myty@users.noreply.github.com>
@myty myty changed the title WIP - Node.js SDK - Refactor CliDownloader Node.js SDK - CLI Downloader Jan 20, 2023
@myty myty marked this pull request as ready for review January 20, 2023 01:18
myty
myty commented Jan 20, 2023
View reviewed changes
Comment thread sdk/nodejs/provisioning/cli-downloader.ts Outdated
myty
myty commented Jan 20, 2023
View reviewed changes
Comment thread sdk/nodejs/provisioning/cli-downloader.ts Outdated
@myty
only download archive is cli version is not already cached
87d7ed7 
Signed-off-by: Michael Tyson <myty@users.noreply.github.com>
@myty myty force-pushed the feature/sdk-nodejs-windows-cli-downloader branch from 1e62b18 to 87d7ed7 Compare January 20, 2023 01:50
@myty
rename download method, update constructor error
2a5bef3 
Signed-off-by: Michael Tyson <myty@users.noreply.github.com>
@myty myty force-pushed the feature/sdk-nodejs-windows-cli-downloader branch from 14fe39e to 2a5bef3 Compare January 21, 2023 02:08
myty added 3 commits January 20, 2023 21:22
@myty
Merge branch 'dagger:main' into feature/sdk-nodejs-windows-cli-downlo…
441007d 
…ader
@myty
update test stub for download method
a178f3a 
Signed-off-by: Michael Tyson <myty@users.noreply.github.com>
@myty
additional cli-downloader refactoring and tests
0cbfea8 
Signed-off-by: Michael Tyson <myty@users.noreply.github.com>
@dolanor dolanor requested a review from slumbering January 23, 2023 12:51
gerhard
gerhard reviewed Jan 24, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@gerhard gerhard left a comment

Choose a reason for hiding this comment

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

Thanks for this PR! We did a quick review with @slumbering & @dolanor. There are a few more things that they will be adding before we can move this along. Our intention is to have this merged by the end of the week.

The biggest thing that we are missing right now is an integration test that gives us confidence in this change. @dolanor @slumbering will follow-up on this.

Thanks @myty 👍

Comment thread sdk/nodejs/provisioning/cli-downloader/cli-downloader.ts Outdated
Comment thread sdk/nodejs/provisioning/cli-downloader/cli-downloader.ts Outdated
dolanor
dolanor suggested changes Jan 24, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@dolanor dolanor left a comment

Choose a reason for hiding this comment

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

Hi @myty,
Thanks for your contribution.

I have 2 questions/suggestions.
I'm also for the integration tests to make sure nothing is broken in between as the change is big and change the logic quite a bit :)

Comment thread sdk/nodejs/connect.ts
Comment thread sdk/nodejs/provisioning/cli-downloader/cli-downloader-factory.ts Outdated
myty and others added 2 commits January 25, 2023 21:56
@myty @gerhard
Update sdk/nodejs/provisioning/cli-downloader/cli-downloader.ts
9859ede 
Co-authored-by: Gerhard Lazu <gerhard@users.noreply.github.com>
Signed-off-by: Michael Tyson <myty@users.noreply.github.com>
@myty
addressed PR feedback
015a712 
Signed-off-by: Michael Tyson <myty@users.noreply.github.com>
dolanor
dolanor reviewed Jan 27, 2023
View reviewed changes
Comment thread sdk/nodejs/connect.ts Outdated
@dolanor
fix: typo
ab12486 
Signed-off-by: Tanguy ⧓ Herrmann <dolanorgit@evereska.org>
@dolanor
Copy link
Copy Markdown
Contributor

dolanor commented Jan 30, 2023

Hi @myty ,
We didn't manage to finish last week.
We spent time with @slumbering today, and I think I found some flaws in the logic and as well in our tests. So we won't merge until we make sure our tests work, and that the implementation works. I hope I can dedicate time this week.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Feb 14, 2023

This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 7 days.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Feb 22, 2023

This PR was closed because it has been stalled for 7 days with no activity.

@github-actions github-actions Bot closed this Feb 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@slumbering slumbering Awaiting requested review from slumbering

2 more reviewers

@gerhard gerhard gerhard left review comments

@dolanor dolanor dolanor requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

kind/stale

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@myty @dolanor @gerhard @slumbering