-
Notifications
You must be signed in to change notification settings - Fork 112
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
Generating fixture internally #457
Conversation
cdf856e
to
8c19cce
Compare
e790852
to
b5f19d7
Compare
https://pulp.plan.io/issues/5872 Required PR: pulp/pulpcore#457 Required PR: pulp/pulp-smash#1228 closes #5872
.travis/before_script.sh
Outdated
cd $TRAVIS_BUILD_DIR/../pulp-operator | ||
.travis/pulp-operator-check-and-wait.sh | ||
cd $TRAVIS_BUILD_DIR/../pulpcore | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these changes specific to pulpcore?
If so, they need to be in PRE_BEFORE_SCRIPT or POST_BEFORE_SCRIPT.
If they need to be in pulpcore, they need to be in the plugin-template. There would be a corresponding PR in plugin-template reviewed at the same time as pulpcore's PR.
https://pulp.plan.io/issues/5872 Required PR: pulp/pulpcore#457 Required PR: pulp/pulp-smash#1228 closes #5872
589b083
to
2a8d2bc
Compare
https://pulp.plan.io/issues/5872 Required PR: pulp/pulpcore#457 Required PR: pulp/pulp-smash#1228 closes #5872
https://pulp.plan.io/issues/5872 Required PR: pulp/pulpcore#457 Required PR: pulp/pulp-smash#1228 closes #5872
208c1ec
to
55dcc6f
Compare
a251068
to
0b3d7cc
Compare
https://pulp.plan.io/issues/5872 Required PR: pulp/pulpcore#457 Required PR: pulp/pulp-smash#1228 closes #5872
https://pulp.plan.io/issues/5872 Required PR: pulp/pulpcore#457 Required PR: pulp/pulp-smash#1228 closes #5872
.travis/pre_before_script.sh
Outdated
access_log $TRAVIS_BUILD_DIR/tmp/access.log; | ||
} | ||
} | ||
NGINXCONF |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any chance we could put this into a separate file inside the .travis
directory?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done!
nginx -c `pwd`/tmp/etc/nginx.conf | ||
|
||
cat .travis/pulp-smash-config.json | \ | ||
jq 'setpath(["custom","fixtures_origin"]; "http://localhost:8000/fixtures/")' > temp.json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you setting fixtures_origin here instead of just setting it in pulp-smash-config.json? Is this to avoid conflicts with plugins?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly, if I declare it in pulp-smash-config.json, it would force all plugins to have local fixtures
This is looking great. Very excited to have this. 👍 |
https://pulp.plan.io/issues/5872 Required PR: pulp/pulpcore#457 Required PR: pulp/pulp-smash#1228 closes #5872
https://pulp.plan.io/issues/5872 Required PR: pulp/pulpcore#457 Required PR: pulp/pulp-smash#1228 closes #5872
ead0459
to
77253c4
Compare
https://pulp.plan.io/issues/5872 Required PR: pulp/pulpcore#457 Required PR: pulp/pulp-smash#1228 closes #5872
https://pulp.plan.io/issues/5872 Required PR: pulp/pulpcore#457 Required PR: pulp/pulp-smash#1228 closes #5872
.travis/nginx.conf
Outdated
autoindex_exact_size off; | ||
} | ||
error_log $TRAVIS_BUILD_DIR/tmp/error.log; | ||
access_log $TRAVIS_BUILD_DIR/tmp/access.log; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick -- the indentation is inconsistent in this file.
https://pulp.plan.io/issues/5873 Required PR: pulp/pulp-smash#1228 closes #5873
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like we should refactor this into an option for plugin-template. Only a few variables would be involved: A boolean generate local fixtures (which affects nginx in .travis.yml) in template_config.yml, blocks of code based in before_script.sh if it is true, and we'd change the pulp-smash-config.json based on the plugin name.
Note that you seemed to indicate that pulp-smash-config.json is part of plugin-template, but I do not see it there.
@@ -35,6 +35,7 @@ addons: | |||
packages: | |||
- httpie | |||
- jq | |||
- nginx |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is the corresponding change to plugin-template?
Without it, it would be blown away by it being run.
pulp-smash-config.json from pulpcore is used in all plugins: |
https://pulp.plan.io/issues/5872 Required PR: pulp/pulpcore#457 Required PR: pulp/pulp-smash#1228 closes #5872
https://pulp.plan.io/issues/5873
closes #5873
Please be sure you have read our documentation on creating PRs:
https://docs.pulpproject.org/en/3.0/nightly/contributing/pull-request-walkthrough.html