Users/dakale/use ruby #10083

dakale · 2019-04-10T19:21:01Z

Adds task for use: ruby

Task has two inputs per design: version, architecture. Both optional.

Version input has no behavior change from existing Use Ruby Version task, will fail if tool is not found in cache

(@damccorm since I cant add reviewer without write access)

Tasks/UseRubyV1/Strings/resources.resjson/de-de/resources.resjson

Tasks/UseRubyV1/useruby.ts

damccorm · 2019-04-10T19:32:33Z

Tasks/UseRubyV1/installer.ts

+export async function installRubyVersion(parameters: TaskParameters): Promise<void> {
+    const toolName: string = "Ruby";
+    const installDir: string | null = tool.findLocalTool(toolName, parameters.version, parameters.architecture);
+    if (!installDir) {


This doesn't make sense to me - why are we throwing if the tool isn't found locally? Shouldn't we download if its not found?

And cache it once we download it.

I guess this is what the existing task is doing, it feels like we should be able to do better than that though.

Double checking with @vtbassmatt

I think the idea was to preserve behavior for now and extend later

Ok - if we do decide to download it looks like the dist is pretty well structured - https://cache.ruby-lang.org/pub/ruby/

Can probably basically just do the same thing we do in the UseNode task

As far as I can tell those are not prebuilt binaries and I doubt we can afford the time to build ruby on demand. Ill dig in a little more, but I believe lack of simple multiplat binary distributions is part of why the existing task doesnt have this functionality

Oh yeah, you're right 😕

I still think its weird that we download other tools when they're not present, but not Ruby. It looks like the plan is to do the same with Python though.

With that said, I don't see a good way around this so I guess I'm ok with it.

Now that this is "done" Ill check with Matt on timelines. I think the on-demand install is a nice-to-have so we may choose to go for it, and see if its viable to get done within the timeline. I agree with you that being consistent would be ideal, and as far as I'm aware there's no licensing issues or anything blocking this.

I would probably say lets go ahead and get this merged if it looks correct, even if we end up immediately revisiting and extending it

In an ideal world, we'd always download any version the customer asked for. In practice that's not always been easy/possible, so we've fallen back to just the tools cache. @dakale is right that we need to at least match parity, making improvements where possible but not blocking on them.

Tasks/UseRubyV1/installer.ts

Tasks/UseRubyV1/package.json

Tasks/UseRubyV1/installer.ts

damccorm · 2019-04-10T19:49:14Z

make-options.json

@@ -140,6 +140,7 @@
        "UseDotNetV2",
        "UseNodeV1",
        "UsePythonVersionV0",
+        "UseRubyV1",


We should probably add this to codeowners as well: https://github.com/Microsoft/azure-pipelines-tasks/blob/master/.github/CODEOWNERS

I don't think it will work with you since you're not a maintainer, you can probably add bryan and me though. Alternately you can request to become a maintainer of the vsts-dt-admin group, Bryan would approve that.

Ill add myself as owner for now to catch any basic issues, then reassign to the proper ecosystem owner once they even are aware of its existence / are familiar with the changes

That's fine - my only concern is will CodeOwners work even though you're not a maintainer? (Genuinely not sure)

Im not sure. Are you aware of any automation that uses this information? I just assumed it was informational, and would serve as a way to route issues to me. It would be surprising if something tried to write to the repo on my behalf based on that info, since I could just put any name there

Does @stephenmichaelf know?

It doesn't write to a repo, but it does make the codeowner a required reviewer. For example, in #9716, Ross was added as a reviewer automatically since he's a codeowner.

https://github.blog/2017-07-06-introducing-code-owners/

I can't find any docs calling out that this only works for maintainers, I think we can probably assume it works for everyone. Worst case it doesn't and we just change it.

Tasks/UseRubyV1/package.json

Remove Strings/ Use task.getPlatform instead of local variation Add package.json to Tests/ Always set task.setResourcePath even if no version

dakale · 2019-04-11T16:57:22Z

Working on build failures. Will update

One build was timing out with 1000ms with no other apparent failure mode

damccorm

Left a couple comments, but overall this seems good.

damccorm · 2019-04-11T21:05:42Z

Tasks/UseRubyV1/Tests/L0.ts

+        assert(tr.stdout.includes('##vso[task.prependpath]' + '/Ruby/2.4.4/bin'), 'ruby tool location was not added to PATH as expected');
+    });
+
+    it('sets PATH correctly on Windows', function () {


Could you add a test to verify proxy behavior?

Sure, ive added a test in 6410e52

Also add preview: true to task.json

…ines-tasks into users/dakale/use-ruby

damccorm · 2019-04-12T16:00:51Z

Looks like a random timeout caused macos to fail, maybe increase the timeout a little bit more?

dakale · 2019-04-12T17:11:16Z

Yea, it looks like the mock-test runner downloads a node10 on the first run: https://github.com/Microsoft/azure-pipelines-task-lib/blob/master/node/mock-test.ts#L160 I only skimmed this code but seems to download node based on the node specified in the task. The test says it is testing against node 6, so I would like to verify that it is actually doing that, or if its using node10 per the task.json. Not going to invest that time right now though

That makes sense as to why the first test is the one thats always failing. For now, I think the easiest workaround is just to increase the timeout as mentioned. We could potentially add a new MockTestRunner() in the before to prime the cache, but that before would need a long timeout too, so only minor gains in terms of keeping the tests cohesive. Although it could potentially boost perf if for some reason all other tests were going to timeout, we would be waiting less time for the N tests to reach the new timeout.

dakale · 2019-04-12T17:17:25Z

For clarity, from the failing logs:

> mocha /Users/vsts/agent/2.150.0/work/1/s/_build/Tasks/UseRubyV1/Tests/L0.js /Users/vsts/agent/2.150.0/work/1/s/_build/Tests/L0.js


  UseRubyVersion L0 Suite
Downloading file: https://nodejs.org/dist/v10.15.1/node-v10.15.1-darwin-x64.tar.gz

    1) finds version in cache in Linux

The test suite header is printed, then a line about downloading, then the result of the test. Its easy to
verify that any output generated by the test is printed before the test failure, assuming no weird async behavior

damccorm · 2019-04-12T17:29:14Z

Its definitely testing against node 10, I'll think more about this timeout issue though. Regardless, that shouldn't be the responsibility of this task, we should probably handle it earlier in the build.

damccorm · 2019-04-12T17:30:11Z

I think this is good to go, I'll go ahead and merge

David Kale added 5 commits April 4, 2019 11:53

Add basic UseRuby files

af6c943

Make tests green. Plumb arch through

12c1dac

WIP Testing

124f3f8

Merge branch 'master' into users/dakale/use-ruby

203a1d7

Bump to 1.0.0, Add Strings/

aec8381

damccorm self-requested a review April 10, 2019 19:24

damccorm reviewed Apr 10, 2019

View reviewed changes

PR Feedback

1b729bb

Remove Strings/ Use task.getPlatform instead of local variation Add package.json to Tests/ Always set task.setResourcePath even if no version

David Kale added 3 commits April 11, 2019 14:59

Fix build

957f18e

Use suite level timeout

2906800

One build was timing out with 1000ms with no other apparent failure mode

Merge branch 'master' into users/dakale/use-ruby

607520e

damccorm approved these changes Apr 11, 2019

View reviewed changes

David Kale added 2 commits April 12, 2019 11:44

Add L0 - Set proxy

6410e52

Also add preview: true to task.json

Merge branch 'users/dakale/use-ruby' of github.com:dakale/azure-pipel…

b259f38

…ines-tasks into users/dakale/use-ruby

Increase test timeout

388f773

Merge branch 'master' into users/dakale/use-ruby

3a8379e

damccorm merged commit 93b96a2 into microsoft:master Apr 12, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Users/dakale/use ruby #10083

Users/dakale/use ruby #10083

dakale commented Apr 10, 2019 •

edited

Loading

damccorm Apr 10, 2019

damccorm Apr 10, 2019

damccorm Apr 10, 2019

dakale Apr 11, 2019

damccorm Apr 11, 2019

dakale Apr 11, 2019

damccorm Apr 11, 2019

dakale Apr 11, 2019

damccorm Apr 11, 2019

vtbassmatt Apr 12, 2019

damccorm Apr 10, 2019

dakale Apr 11, 2019

damccorm Apr 11, 2019

dakale Apr 12, 2019

damccorm Apr 12, 2019

damccorm Apr 12, 2019

dakale commented Apr 11, 2019

damccorm left a comment

damccorm Apr 11, 2019

dakale Apr 12, 2019

damccorm commented Apr 12, 2019

dakale commented Apr 12, 2019

dakale commented Apr 12, 2019

damccorm commented Apr 12, 2019

damccorm commented Apr 12, 2019

Users/dakale/use ruby #10083

Users/dakale/use ruby #10083

Conversation

dakale commented Apr 10, 2019 • 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

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

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

dakale commented Apr 11, 2019

damccorm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damccorm commented Apr 12, 2019

dakale commented Apr 12, 2019

dakale commented Apr 12, 2019

damccorm commented Apr 12, 2019

damccorm commented Apr 12, 2019

dakale commented Apr 10, 2019 •

edited

Loading