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.

Sign up for GitHub

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

Jump to bottom

Fix DockerInstallerV0 for ARM agent #13199

Merged
merged 2 commits into from
Jun 30, 2020
Merged

Fix DockerInstallerV0 for ARM agent #13199

merged 2 commits into from
Jun 30, 2020

Conversation

winromulus
Copy link
Contributor

Task name: DockerInstallerV0

Description: Added case for armhf based docker installer

Documentation changes required: N

Added unit tests: N

Attached related issue: #13198

Checklist:

  • Task version was bumped - please check instruction how to do it
  • Checked that applied changes work as expected

@winromulus
Fix DockerInstallerV0 for ARM agent
8209622 
Added case for `armhf` based docker installer
Fixes: #13198
Fix version
@ghost
Copy link

ghost commented Jun 29, 2020 •
edited by ghost
Loading

CLA assistant check
All CLA requirements met.

@thesattiraju thesattiraju self-requested a review June 30, 2020 04:48
thesattiraju
thesattiraju suggested changes Jun 30, 2020
View reviewed changes
Tasks/DockerInstallerV0/task.json Show resolved Hide resolved
Tasks/DockerInstallerV0/src/utils.ts Outdated Show resolved Hide resolved
thesattiraju
thesattiraju suggested changes Jun 30, 2020
View reviewed changes
Copy link
Contributor

@thesattiraju thesattiraju left a comment

Choose a reason for hiding this comment

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

We're almost there.. could you add a test for the same?

@winromulus
Copy link
Contributor Author

@DS-MS I would very much like to add a test but this is beyond my current ramp-up on the azure-pipelines-tasks framework.
I've looked into the tests file and there's a big TODO in there (no tests at all). If anyone else would like to contribute with that, I'd be happy to follow along. I've looked into the other installers (helm and kubectl) for sample tests but couldn't find any.
It would have been great if a test was added when arm64 was introduced so I had something to follow as an example of the framework.

My fix is is for adding a missing option for an architecture which is marked as "supported".

@thesattiraju
Copy link
Contributor

Oh! My bad. I just saw the L0.ts file to be present and expected the test framework to be setup correctly, already.

Thanks for the contribution. It LGTM; I'll take it forward. 😄

thesattiraju
thesattiraju approved these changes Jun 30, 2020
View reviewed changes
Copy link
Contributor

@thesattiraju thesattiraju left a comment

Choose a reason for hiding this comment

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

LGTM

@thesattiraju thesattiraju merged commit 3106c9f into microsoft:master Jun 30, 2020
@thesattiraju
Copy link
Contributor

The rollout for this would start next Monday.. should reach all the rings within the next 3 weeks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thesattiraju thesattiraju thesattiraju approved these changes

@ammohant ammohant Awaiting requested review from ammohant

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@winromulus @thesattiraju