Add returncode to dcos_cli.exec_command

[Fabs]

High-level description

This enables test using dcos_cli to assert the returncode of the CLI process. We will use that specifically on integration tests for Edge-LB.

I did not include an integration test be# Corresponding DC/OS tickets (obligatory)

These JIRA ticket(s) must be updated (ideally closed) in the moment this PR lands:

• DCOS-42411 Refactoring and extending the Edge-LB integration tests
• DCOS-42678 Pytest Fixtures for edgelb-cli

Related tickets (optional)

Related dcos-launch and dcos PRs

Is this change going to be propagated up into another repo? Test the change by bumping the dcos-test-utils SHA to point to these changes to test it. Link the corresponding PRs here:

Checklist for all PRs

• Included a test which will fail if code is reverted but test is not. If there is no test please explain here:
• Include a test in dcos-integration-tests in https://github.com/dcos/dcos or explain why this is not applicable:
• Include a test in https://github.com/dcos/dcos-launch or explain why this is not applicable:

Integration tests were run and

• Integration Test - Enterprise (link to job: )
• Integration Test - Open (link to job: )

PLEASE FILL IN THE TEMPLATE ABOVE / DO NOT REMOVE ANY SECTIONS ABOVE THIS LINE

Instructions and review process

What is the review process and when will my changes land?

All PRs require 2 approvals using GitHub's pull request reviews.

Reviewers should be:

• Developers who understand the code being modified.
• Developers responsible for code that interacts with or depends on the code being modified.

It is best to proactively ask for 2 reviews by @mentioning the candidate reviewers in the PR comments area. The responsibility is on the developer submitting the PR to follow-up with reviewers and make sure a PR is reviewed in a timely manner.

[vespian]

❤️

[Fabs]

I tried to rebuild the tox-integration @cprovencher, and I also looked at the failure. It would be cool if you could give some insight on why you think the tests is failing.

[cprovencher]

@Fabs looks like it was a flake related to dcos-launch and not your changes

[Fabs]

Thanks, @cprovencher. What is still missing so we could merge it? I am not authorized to merge.

[Fabs]

@cprovencher. What do I need to do to get this merged?

[adamtheturtle]

@Fabs This is a backwards incompatible change.
Merging this would mean that DC/OS Enterprise integration tests could not bump DC/OS Test Utils.

See https://github.com/mesosphere/dcos-enterprise/blob/864cbb3e745945558e2315563b8d5a2c4b347292/packages/dcos-integration-test/extra/test_dcoscli_enterprise.py#L18 for an example of this.

Maybe I missed it, but I cannot see a JIRA issue which tracks updating existing consumers. I consider this responsibility one of the author of a backwards-incompatible change. Therefore, I have rejected this change.

What I'd expect to approve:

• A JIRA issue to update DC/OS Enterprise which updates the version of DC/OS Test Utils
• A documented investigation into which other consumers will suffer from this change. That is, which teams will see their CI break when they bump DC/OS Test Utils.
• A commitment to do the above JIRA issues immediately after merging - preferably before merging really with bumps that look at your branch

Happy to discuss.

[Fabs]

@adamtheturtle thanks for spotting that. I also had a quick conversation with @cprovencher and @orsenthil I think your approach is the right thing to do. Be back with the requested changes/docs/tickets.

[cprovencher]

@adamtheturtle If all consumers are using a pinned version there shouldn't be any issue?

[adamtheturtle]

@cprovencher You are correct in that this means that no CI will immediately break. That's why my request was for @Fabs to look into "which teams will see their CI break when they bump DC/OS Test Utils.".

I also wrote "I consider this responsibility one of the author of a backwards-incompatible change" and "Happy to discuss." as this is not a documented requirement of any backwards-incompatible changes, but to me it is poor practice to put the burden on other teams.

[Fabs]

@adamtheturtle and @cprovencher. I had a conversation with @orlandohohmeier and we can take on the measures that we discussed. There is one extra step we would like to take, please let me know what you think of the full plan:

• Introduce a new method for exec that returns the process object entirely, mark the exec_command as deprecated and make it use call exec and convert the process to the pair (stdout, stderr). We could merge that and SHIPT IT
• Provide a PR for dcos removing uses of the deprecated method
• Provide a PR for dcos-enterprise removing uses of the deprecated method
• Provide a PR for dcos-kubernetes removing uses of the deprecated method
• Provide a PR for dcos-commons removing uses of the deprecated method
• Notify all projects about the deprecation (maybe just a shout at eng-all). The projects come form this query https://github.com/search?p=1&q=dcos-test-utils&type=Code. Good news, they are almost pinned
• Open a PR to remove the deprecated method

If you have no objections I will proceed creating tickets in an EPIC, and I would own the epic.

cc/ @vespian and @orsenthil

[adamtheturtle]

@Fabs First thought - great ❤️

[Fabs]

@adamtheturtle, we never came back to that. We are already using this version on the edge-lb tests. What shall we do to merge this first step?

[Fabs]

Ups, sorry @adamtheturtle, the pr is actually this one - #74