-
Notifications
You must be signed in to change notification settings - Fork 4
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
config: automatic manifest generation #18
config: automatic manifest generation #18
Conversation
3fcfe4d
to
64b5ae0
Compare
64b5ae0
to
15c822e
Compare
.gitignore
Outdated
|
||
# Built config | ||
configuration_manifests/ |
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.
configuration-manifests
as we usually use dashes instead of underscores?
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.
👍
@@ -0,0 +1,126 @@ | |||
--- | |||
# Deployment [local|prod] |
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.
This file could be documented (via comments) just a little bit more and then included under "Configuration" section in the documentation. Shall we do it now or later?
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.
Maybe we should do it in a separate PR addressing reanahub/reana#4 so we have the functionality shipped faster, WDYT?
scripts/build-manifests.sh
Outdated
@@ -0,0 +1,47 @@ | |||
#!/bin/bash | |||
|
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.
The scripts misses license statement. There are several shell script improvements to do, such as:
- do not use
$1
throughout the file, rather read the arguments in the beginning into variables and use those; - semicolons at the end of statements are not needed if there is no follow-up statement to execute
- see also
shellchech
recommendations such ascd $somedir || exit
Shall we do these now or via a later cleanup?
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.
I think this can be done now, so we do not have to change it until we create the Python CLI.
setup.py
Outdated
@@ -84,6 +84,7 @@ | |||
extras_require=extras_require, | |||
setup_requires=setup_requires, | |||
tests_require=tests_require, | |||
scripts=['scripts/build-manifests.sh'], |
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.
Do we need to install this script? If it gets installed under /some/prefix/bin/build-manifests.sh
, then we should perhaps think of a name like reana-build-manifests
... Let's muse IRL.
1d16fac
to
d48d8db
Compare
* (addresses reanahub/reana#13) Signed-off-by: Diego Rodriguez <[email protected]>
Signed-off-by: Diego Rodriguez <[email protected]>
* Since config for namespaces, quotas and workers are stored in a single jinja template, the output was a single file with many yaml files inside. This commit generates a file for each yaml. Signed-off-by: Diego Rodriguez <[email protected]>
* In order to make `build-manifests.sh` accesible when installing the package, both the script and the `templates` directory have been added to it. Signed-off-by: Diego Rodriguez <[email protected]>
Signed-off-by: Diego Rodriguez <[email protected]>
Signed-off-by: Diego Rodriguez <[email protected]>
Signed-off-by: Diego Rodriguez <[email protected]>
Signed-off-by: Diego Rodriguez <[email protected]>
Signed-off-by: Diego Rodriguez <[email protected]>
Signed-off-by: Diego Rodriguez <[email protected]>
38639a3
to
221a3b9
Compare
docs/gettingstarted.rst
Outdated
|
||
Once we have done it, we should be able to see the directory `configuration-manifests` with the following structure: | ||
|
||
.. code-block:: sh |
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.
Since you are showing more than just a shell script source, it's better to use code-block:: console
here. (And everywhere where you show a console session, i.e. shell commands and their output.)
docs/gettingstarted.rst
Outdated
|
||
.. code-block:: sh | ||
|
||
$ python setup.py install |
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.
We can be ready towards future release by installation instructions going like:
First, install latest stable release of `reana-resources-k8` package:
pip install reana-resources
or use development version:
git clone ...
cd ...
pip install .
Second, you can build resource manifests by doing...
...
scripts/reana-resources-k8s
Outdated
@@ -0,0 +1,73 @@ | |||
#!/bin/sh | |||
|
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.
Please add copyright header information, e.g. take it from .travis.yml
.
scripts/reana-resources-k8s
Outdated
template_file_name=$(printf '%s' "$template_file_path" | sed -e "s@$templates_path\/@@g") | ||
config_file_name=$(printf '%s' "$template_file_name" | sed -e "s/-template//g") | ||
config_file_path="$templates_path/../configuration-manifests/$config_file_name" | ||
jinja2 "$template_file_path" "$templates_path/config.yaml" > "$config_file_path" |
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.
jinja2-cli
is missing in setup.py
as a required dependecy. Please add it.
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.
... ditto pyyaml
is missing. Please add.
docs/gettingstarted.rst
Outdated
.. code-block:: sh | ||
|
||
$ python setup.py install | ||
$ reana-resources-k8s build-manifests |
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.
Please add reana-resources-k8s build-manifests
step to the Travis CI building instructions. Otherwise we won't catch errors in shell scripts... (such as missing pyyaml
dependency I talk about elsewhere).
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.
BTW shall we use "build-manifests" terminology? Wondering because we usually use "create-instance" etc...
docs/configuration.rst
Outdated
============= | ||
|
||
|
||
Configuration for the REANA system can be made through the file `config.yaml` under `templates`: |
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.
Better use a language iike:
REANA-Resources-K8 default congifuration is defined in templates/config.yaml
file. It is printed below for inspiration. You can define your own myconfig.yaml
and then use reana-resources-k8 build-manifests myconfig.yaml
to fit your computing cloud. (and link to CLI documentation)
Signed-off-by: Diego Rodriguez <[email protected]>
Signed-off-by: Diego Rodriguez <[email protected]>
Signed-off-by: Diego Rodriguez <[email protected]>
Signed-off-by: Diego Rodriguez <[email protected]>
2834df2
to
25b5509
Compare
Signed-off-by: Diego Rodriguez <[email protected]>
Signed-off-by: Diego Rodriguez [email protected]