Skip to content

/ saltfs

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

Validate Homu TOML file #420

Merged
merged 1 commit into from Jul 11, 2016
Merged

Validate Homu TOML file #420

merged 1 commit into from Jul 11, 2016

Conversation

@Crimack
Copy link
Contributor

Crimack commented Jul 1, 2016 •
edited by larsbergstrom

Draft of resolution to #407. config_path in sls/homu/toml.py is definitely wrong and should be corrected

This change is Reviewable
@Crimack
Copy link
Contributor Author

Crimack commented Jul 1, 2016

@aneeshusa I think I see what I've done wrong. I should be checking the generated TOML file at /etc/init/homu.conf on servo-master1 rather than the Jinja template, right?
@Crimack Crimack changed the title Initial attempt to solve issue 407 Validate Homu TOML file Jul 1, 2016
@aneeshusa
Copy link
Member

aneeshusa commented Jul 1, 2016

Correct!
@Crimack Crimack force-pushed the Crimack:407_valid_toml branch 3 times, most recently from 29d0563 to 7a77865 Jul 1, 2016
tests/sls/homu/toml.py Outdated
config_path = os.path.join('/home', 'servo', 'homu', 'cfg.toml')
with open(config_path) as conf:
try:
toml.loads(conf.read())

This comment has been minimized.

Sign in to view
@aneeshusa

aneeshusa Jul 3, 2016

Member

We can use toml.load instead which will handle opening and closing the file for us.
tests/sls/homu/toml.py Outdated
with open(config_path) as conf:
try:
toml.loads(conf.read())
return Success('Homu config file is valid TOML')

This comment has been minimized.

Sign in to view
@aneeshusa

aneeshusa Jul 3, 2016

Member

Pull this out of the try block and have it as the last statement in run().
tests/sls/homu/toml.py Outdated
return Success('Homu config file is valid TOML')
except Exception as e:
return Failure('Homu config file is not valid TOML: ',
'{}'.format(e))

This comment has been minimized.

Sign in to view
@aneeshusa

aneeshusa Jul 3, 2016

Member

This can be str(e).

This comment has been minimized.

Sign in to view
@aneeshusa

aneeshusa Jul 3, 2016

Member

Also, this should then fit on the previous line.
tests/sls/homu/toml.py Outdated
toml.loads(conf.read())
return Success('Homu config file is valid TOML')
except Exception as e:
return Failure('Homu config file is not valid TOML: ',

This comment has been minimized.

Sign in to view
@aneeshusa

aneeshusa Jul 3, 2016

Member

No need for the space after the colon. (The output will start from the next line).
tests/sls/homu/toml.py Outdated


def run():
config_path = os.path.join('/home', 'servo', 'homu', 'cfg.toml')

This comment has been minimized.

Sign in to view
@aneeshusa

aneeshusa Jul 3, 2016

Member

It looks strange to see os.path.join when we have an explicit / in the arguments. Because I don't forsee us ever running Homu on a non-Unix platform, and because this is also hardcoded in the homu/init.sls file, let's just hardcode the whole path and inline it where it gets used.
requirements.txt Outdated
@@ -1,3 +1,4 @@
# Python modules needed for testing
# (Travis-CI will auto-install them from this file)
flake8 == 2.5.4
toml == 0.9.1

This comment has been minimized.

Sign in to view
@aneeshusa

aneeshusa Jul 3, 2016

Member

Please also pin this in homu/init.sls, and put a comment in both places that mentions the need to keep them in sync. See the style guide for tips.
@Crimack
Copy link
Contributor Author

Crimack commented Jul 4, 2016

I think that's all your comments cleared up. Thank you for the feedback by the way, it was very easy to follow!

The additions to homu/init.sls might be wrong though? I wasn't sure if I should make a new 'block' (not sure what the proper SaltStack term for this is), or add it to one of the existing ones.
homu/init.sls Outdated
toml:
pip.installed:
- pkgs:
- toml == 0.9.1 # Ensure this is up to date with requirements.txt

This comment has been minimized.

Sign in to view
@aneeshusa

aneeshusa Jul 5, 2016

Member

Instead of a separate Salt state (block), add this to the pip.installed state above that installed Homu. Also, let's say 'in sync' instead of 'up to date'.

This comment has been minimized.

Sign in to view
@Crimack

Crimack Jul 5, 2016

Author Contributor

Done :)
requirements.txt Outdated
@@ -1,3 +1,4 @@
# Python modules needed for testing
# (Travis-CI will auto-install them from this file)
flake8 == 2.5.4
toml == 0.9.1 # Please ensure this is also in homu/init.sls

This comment has been minimized.

Sign in to view
@aneeshusa

aneeshusa Jul 5, 2016

Member

'is also in' -> 'version is in sync with'
.travis/dispatch.sh Outdated
@@ -52,6 +52,6 @@ else
# Only run tests against the new configuration
# TODO: don't hard-code this
if [ "${SALT_NODE_ID}" = "servo-master1" ]; then
./test.py sls.buildbot.master sls.nginx
./test.py sls.buildbot.master sls.nginx sls.homu

This comment has been minimized.

Sign in to view
@aneeshusa

aneeshusa Jul 6, 2016

Member

Let's keep these in alphabetical order, so insert sls.homu in between sls.buildbot.master and sls.nginx.
@aneeshusa
Copy link
Member

aneeshusa commented Jul 6, 2016

LGTM aside from that last comment, so go ahead and squash when you address that.
@Crimack Crimack force-pushed the Crimack:407_valid_toml branch from 49225d4 to 5e284fa Jul 7, 2016
@Crimack
Copy link
Contributor Author

Crimack commented Jul 7, 2016

All good now I think!
@Crimack
Added test to check if homu.conf is valid TOML
Loading status checks…
5e284fa
@aneeshusa
Copy link
Member

aneeshusa commented Jul 11, 2016

Checked this locally in Vagrant and everything looks good. @bors-servo r+
@bors-servo
Copy link
Contributor

bors-servo commented Jul 11, 2016

📌 Commit 5e284fa has been approved by aneeshusa
@bors-servo
Copy link
Contributor

bors-servo commented Jul 11, 2016

⌛ Testing commit 5e284fa with merge 4d25967...
bors-servo added a commit that referenced this pull request Jul 11, 2016
@bors-servo
Auto merge of #420 - Crimack:407_valid_toml, r=aneeshusa
Loading status checks…
4d25967 
Validate Homu TOML file

Draft of resolution to #407. `config_path` in sls/homu/toml.py is definitely wrong and should be corrected

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

bors-servo commented Jul 11, 2016

☀️ Test successful - travis
@bors-servo bors-servo merged commit 5e284fa into servo:master Jul 11, 2016
2 of 3 checks passed
2 of 3 checks passed
code-review/reviewable 5 files, 9 discussions left (aneeshusa, Crimack)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
This was referenced Jul 11, 2016
Merged
Closed
@aneeshusa aneeshusa mentioned this pull request Jul 11, 2016
Closed
@Crimack Crimack deleted the Crimack:407_valid_toml branch Jul 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
@Crimack @aneeshusa @bors-servo
You can’t perform that action at this time.