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.

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

Remove old DynamicServoFactory which reads from saltfs #504

Merged
merged 1 commit into from Oct 19, 2016

Conversation

@aneeshusa
Copy link
Member

aneeshusa commented Oct 6, 2016

Now that the new dynamic factory is working (which reads the steps
configuration from the main servo repo), remove the old one.
However, re-use the old name (DynamicServoFactory instead of
DynamicServoYAMLFactory) for concision and clarity.

Fixes #316. Blocked on servo/servo#13611.


This change is Reviewable

@aneeshusa
Copy link
Member Author

aneeshusa commented Oct 6, 2016

@aneeshusa
Copy link
Member Author

aneeshusa commented Oct 6, 2016

FYI I did this at 2am, so I'd like to take another look in the morning :)

@aneeshusa aneeshusa force-pushed the aneeshusa:always-use-steps-from-servo-repo branch from a0f1027 to b708f18 Oct 6, 2016
@aneeshusa
Copy link
Member Author

aneeshusa commented Oct 6, 2016

servo/servo#13611 has merged, so this is no longer blocked.

@aneeshusa
Copy link
Member Author

aneeshusa commented Oct 12, 2016

steps.yml was changed, now blocked on a follow-up PR.

@bors-servo
Copy link
Contributor

bors-servo commented Oct 12, 2016

The latest upstream changes (presumably #503) made this pull request unmergeable. Please resolve the merge conflicts.

@jdm
Copy link
Member

jdm commented Oct 18, 2016

The followup PR merged. It would be really great to get this reviewed and throw the switch before we change steps.yml again.

Now that the new dynamic factory is working (which reads the steps
configuration from the main servo repo), remove the old one.
However, re-use the old name (DynamicServoFactory instead of
DynamicServoYAMLFactory) for concision and clarity.
@aneeshusa aneeshusa force-pushed the aneeshusa:always-use-steps-from-servo-repo branch from b708f18 to 4075ac4 Oct 18, 2016
@larsbergstrom
Copy link
Contributor

larsbergstrom commented Oct 19, 2016

This looks good. I think we can land this (even though it'll bitrot my MSVC PR). @aneeshusa Is this good to go?

My only worry is that now to add/change builders we have to do coordinated commits AND buildbot restarts across servo/servo and servo/saltfs and that's certainly going to cause breakage every time we have to do it.

@aneeshusa
Copy link
Member Author

aneeshusa commented Oct 19, 2016

Yes, this should be ready. This will optimize for the common case (changing steps for pre-existing builders), and I don't think having to make an extra PR to add/remove a builder (less common) is too onerous.

@larsbergstrom
Copy link
Contributor

larsbergstrom commented Oct 19, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Oct 19, 2016

📌 Commit 4075ac4 has been approved by larsbergstrom

@bors-servo
Copy link
Contributor

bors-servo commented Oct 19, 2016

Test exempted - status

@bors-servo bors-servo merged commit 4075ac4 into servo:master Oct 19, 2016
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test exempted
Details
bors-servo added a commit that referenced this pull request Oct 19, 2016
…rsbergstrom

Remove old DynamicServoFactory which reads from saltfs

Now that the new dynamic factory is working (which reads the steps
configuration from the main servo repo), remove the old one.
However, re-use the old name (DynamicServoFactory instead of
DynamicServoYAMLFactory) for concision and clarity.

Fixes #316. Blocked on servo/servo#13611.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/saltfs/504)
<!-- Reviewable:end -->
@larsbergstrom
Copy link
Contributor

larsbergstrom commented Oct 19, 2016

This has been deployed.

@larsbergstrom
Copy link
Contributor

larsbergstrom commented Oct 19, 2016

Oh, hrm, should we have removed steps.yml from saltfs, too?

@aneeshusa aneeshusa mentioned this pull request Oct 19, 2016
3 of 3 tasks complete
@aneeshusa
Copy link
Member Author

aneeshusa commented Oct 19, 2016

I made #513 for that, good call.

@larsbergstrom
Copy link
Contributor

larsbergstrom commented Oct 19, 2016

What's the easiest way to back this out via salt? The salt-run stuff that I did to stage the new mac?

@aneeshusa
Copy link
Member Author

aneeshusa commented Oct 19, 2016

If #514 doesn't work, easiest is probably to git revert the commits from before this PR up to current master, run a regular highstate, and prepare another PR (with a test builder on Windows!) at our leisure.

The other option is using the override folder and checking out an old git revision, notes at https://github.com/servo/saltfs/blob/b64ff04d219d1fe34c653ae5939f43fc4311d30b/salt/files/master/ADMIN_README.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.