Skip to content
This repository was archived by the owner on Jan 30, 2020. It is now read-only.

tests: added template + metadata functional test#1512

Merged
jonboulle merged 4 commits intocoreos:masterfrom
endocode:kayrus/template_specifiers
Mar 23, 2016
Merged

tests: added template + metadata functional test#1512
jonboulle merged 4 commits intocoreos:masterfrom
endocode:kayrus/template_specifiers

Conversation

@kayrus
Copy link
Contributor

@kayrus kayrus commented Mar 21, 2016

Covers #1446

@kayrus kayrus force-pushed the kayrus/template_specifiers branch from f7ea93b to eca5000 Compare March 21, 2016 21:30
@kayrus
Copy link
Contributor Author

kayrus commented Mar 22, 2016

@jonboulle reverting of PR #1376 resolved the issue. We can keep these tests and merge this PR. Or I can spend more time and fix initial PR. What is your decision?

@jonboulle
Copy link
Contributor

#1376 did not make it into a release right? If not, let's revert that + add the test for this release, and then we can rework a proper version for the next release. Thanks!

@kayrus kayrus changed the title [WIP] tests: added template + metadata functional test tests: added template + metadata functional test Mar 22, 2016
@kayrus
Copy link
Contributor Author

kayrus commented Mar 22, 2016

@jonboulle no, it didn't. tests cover the bug. new #1514 issue has been created. waiting for LGTM ;)

ndesired := 3
if stdout, stderr, err := cluster.Fleetctl(m0, "list-units", "--no-legend", "--full", "--fields", "unit,active,machine"); err != nil {
t.Fatalf("Unable to get submited units: \nstdout: %s\nstderr: %s\nerr: %v", stdout, stderr, err)
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove the else - Fatal will exit anyway

@kayrus
Copy link
Contributor Author

kayrus commented Mar 22, 2016

@jonboulle Done.

@tixxdz
Copy link
Contributor

tixxdz commented Mar 23, 2016

@jonboulle I discussed this with @kayrus, so maybe spend some time on it? cause it's really an optimization boost, previous logic triggered unnecessary RPC and I/O work. The code that can be fixed was located, and there is already a functional test for it, otherwise I'm ok.

@antrik
Copy link
Contributor

antrik commented Mar 23, 2016

@tixxdz I think your last comment should rather go into the newly opened issue, #1514 ? I was quite confused what it refers to at first...

@jonboulle
Copy link
Contributor

I think you're right.
@kayrus thanks, this is great!

@jonboulle jonboulle merged commit dc39755 into coreos:master Mar 23, 2016
@antrik
Copy link
Contributor

antrik commented Mar 23, 2016

@kayrus some formal nitpicks: first of all, please update the title of this PR -- it doesn't apply anymore, and it's very confusing...

I think the new tests should be applied after the revert, to preserve bisectability.

Also, this might be a matter of taste -- but I believe it would be better to have only one revert commit covering both of the original commits (i.e. using revert -n); and with a proper commit message explaining why they are reverted...

@antrik
Copy link
Contributor

antrik commented Mar 23, 2016

Meh, too late ;-)

@jonboulle
Copy link
Contributor

Sorry, all fair comments, just wanted to unblock.

On Wed, Mar 23, 2016 at 4:09 PM Olaf Buddenhagen [email protected]
wrote:

@kayrus https://github.com/kayrus some formal nitpicks: first of all,
please update the title of this PR -- it doesn't apply anymore, and it's
very confusing...

I think the new tests should be applied after the revert, to preserve
bisectability.

Also, this might be a matter of taste -- but I believe it would be better
to have only one revert commit covering both of the original commits (i.e.
using revert -n); and with a proper commit message explaining why they
are reverted...


You are receiving this because you modified the open/close state.

Reply to this email directly or view it on GitHub
#1512 (comment)

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants