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

Add test that generated `cfg.toml` for Homu is a valid TOML file #407

Closed
aneeshusa opened this issue Jun 27, 2016 · 3 comments
Closed

Add test that generated `cfg.toml` for Homu is a valid TOML file #407

aneeshusa opened this issue Jun 27, 2016 · 3 comments

Comments

@aneeshusa
Copy link
Member

@aneeshusa aneeshusa commented Jun 27, 2016

Buildbot has a checkconfig command to check that the master configuration file is valid, which we test in tests/sls/buildbot/master/config.py. AFAIK there is no similar command for Homu yet, but it should be possible to check that the cfg.toml file parses as TOML at the least using a Python TOML library. This should help prevent issues like the one addressed in #405.

A more comprehensive alternative that requires more work is creating such a checkconfig command for Homu, and writing a test that uses that instead.

Files:

  • Create the tests/sls/homu/ folder and make a new Python file inside with the test. Also include an empty __init__.py file in that folder.
  • Add a pinned version of the toml library to the requirements.txt file, so it is available on Travis. This is the library used by Homu; ideally, this is pinned to the same version that Homu uses.
  • Update the .travis/dispatch.sh file to run the new test on the servo-master1 node.
@Crimack
Copy link
Contributor

@Crimack Crimack commented Jun 30, 2016

Hey, I've made an attempt at doing this on a fork by using a Python library. Two problems though:

  1. I can't actually get the Vagrant file to run so I can test? Maybe I'm doing something silly. I keep getting 'extract_id': undefined method 'split' for ["SALT_NODE_ID=servo-linux-cross1", "SALT_FROM_SCRATCH=true"]:Array (NoMethodError) when I try to run vagrant up
  2. Is the homu/files/cfg.toml in the master branch a valid TOML file? I think I've written the test correctly, but it keeps failing.

This is my first pull request to a public repo, so thank you very much for the detailed description of what you wanted done! Made making a first contribution pretty easy :)

@aneeshusa
Copy link
Member Author

@aneeshusa aneeshusa commented Jul 1, 2016

Welcome to Servo!

  1. The Vagrantfile is actually broken (again!), this is my fault 😅. I'm working on a PR to add a test for it so I don't break it again, but in the meantime the changes in this commit should get you up and running. (You don't need the is_salt_master function.)
  2. The homu/files/cfg.toml file in the repo is not valid TOML; we are running it through the Jinja template engine to reduce some boilerplate. However, the end result which is deployed by Salt needs to be a valid TOML file, so you should check that file in the test.

Please go ahead and open a PR with your changes, even if incomplete, to make it easier to provide review comments; we can continue the discussion there.

@aneeshusa aneeshusa added the C-assigned label Jul 1, 2016
bors-servo added a commit that referenced this issue Jul 11, 2016
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 -->
@aneeshusa
Copy link
Member Author

@aneeshusa aneeshusa commented Jul 11, 2016

Fixed by #420.

@aneeshusa aneeshusa closed this Jul 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.