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

Changing DB location for Homu #476

Merged
merged 1 commit into from Apr 22, 2017
Merged

Changing DB location for Homu #476

merged 1 commit into from Apr 22, 2017

Conversation

Coder206
Copy link
Contributor

@Coder206 Coder206 commented Sep 11, 2016

Here is some code that attempts to solve #456.

cc @aneeshusa


This change is Reviewable

@@ -167,3 +167,7 @@ context = 'continuous-integration/appveyor/branch'

[repo.rust-mozjs.status.appveyor]
context = 'continuous-integration/appveyor/branch'

# The database homu uses
Copy link
Contributor

Choose a reason for hiding this comment

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

Unnecessary.

@aneeshusa
Copy link
Contributor

Please configure your editor to ensure files are saved with a trailing newline.

homu/init.sls Outdated

/var/homu:
file.directory:
- source: salt://{{ tpldir }}/var/homu
Copy link
Contributor

Choose a reason for hiding this comment

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

We just want the directory to exist, so no need for a source since we're not filling it with anything.

@aneeshusa
Copy link
Contributor

Thanks for the PR, @Coder206!

@aneeshusa aneeshusa self-assigned this Sep 12, 2016
@aneeshusa
Copy link
Contributor

Reminder to self: demonstrate the salt['file.dirname'] wizardry once the PR is working.

@Coder206
Copy link
Contributor Author

@aneeshusa I made the changes you requested except for adding a template for Salt, I was unsure how to do it.

@@ -167,3 +167,6 @@ context = 'continuous-integration/appveyor/branch'

[repo.rust-mozjs.status.appveyor]
context = 'continuous-integration/appveyor/branch'

[db]
file = "/var/homu/main.db"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aneeshusa Is this correction related to your self-reminder?

@aneeshusa
Copy link
Contributor

@Coder206 Let me know if you want some more tips before trying to get the templating working again (everything else looks good).

If you don't feel comfortable with it, do you mind giving me push access to your branch (https://github.com/blog/2247-improving-collaboration-with-forks)? I can push some commits on top of your branch to show you how it's done, which may be easier than explaining. (Also, if you want to do this, please squash your commits beforehand.)

@Coder206
Copy link
Contributor Author

Coder206 commented Sep 12, 2016

@aneeshusa Yes (I am not very comfortable with doing the template of Salt), I would like a demonstration, give me a minute to send the new changes. I already allowed edits from maintainers.

homu/init.sls Outdated

/var/homu:
file.directory:
- template: jinja
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this, as file.directory can't be templated (there's no content).

@aneeshusa
Copy link
Contributor

Looking good so far. Please squash these into one commit and I will add new commits to this branch to demonstrate templating.

@aneeshusa
Copy link
Contributor

@Coder206 sorry about the review delay on this one, our infra has been a bit crazy recently so I'll get to this in a bit when its more quiet.

@bors-servo
Copy link
Contributor

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

@KiChjang
Copy link
Contributor

@Coder206 Please remove the merge commit.

@aneeshusa
Copy link
Contributor

Hi @Coder206, I've added a commit implementing Salt templating. Please rebase this on top of master and squash the commits!

@Coder206
Copy link
Contributor Author

Coder206 commented Apr 19, 2017

@aneeshusa Sorry for the delays, rebased! Thanks for your help :)

@Coder206
Copy link
Contributor Author

@aneeshusa I made a git mistake, please take a look at the PR again.

@Coder206
Copy link
Contributor Author

@aneeeshua Should I rebase with your changes?

@aneeshusa
Copy link
Contributor

@Coder206 It looks like you dropped the commit I added, I pushed it to your branch again. Please squash again:

git checkout homuDB
git pull
git rebase -i --autosquash @~~

This makes it harder to accidentally delete the database.
@Coder206
Copy link
Contributor Author

@aneeshusa Should I force push the squashed commit?

@aneeshusa
Copy link
Contributor

@Coder206 yes please, I forgot to list that step! I'm also on IRC if you need more git help

@aneeshusa
Copy link
Contributor

Looks great @Coder206, thanks for this!

@bors-servo r+

@bors-servo
Copy link
Contributor

📌 Commit 656ed2b has been approved by aneeshusa

@bors-servo
Copy link
Contributor

⌛ Testing commit 656ed2b with merge d77ca7a...

bors-servo pushed a commit that referenced this pull request Apr 22, 2017
Changing DB location for Homu

Here is some code that attempts to solve #456.

cc @aneeshusa

<!-- 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/476)

<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

☀️ Test successful - status-travis
Approved by: aneeshusa
Pushing d77ca7a to master...

@aneeshusa
Copy link
Contributor

FYI @larsbergstrom @metajack @edunham (and anyone else who deploys), this requires manual intervention when deploying to move the Homu database to the new location.

@Coder206 Coder206 deleted the homuDB branch April 23, 2017 13:50
@larsbergstrom
Copy link
Contributor

I believe that this got deployed, as I had to manually synchronize each of the repositories.

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

Successfully merging this pull request may close these issues.

None yet

6 participants