-
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
Switch to new Galaxy-integrated subdomains and themes #11
Conversation
files | ||
└── galaxy | ||
├── static | ||
│ └── climate |
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.
Could you maybe add a second subdomain here so it's more obvious?
Q: would it not be better for contributors to reverse the logic and have top-level climate
with sub-dirs like static
, themes
.... this way we only request one folder from subdomain maintainers.
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 had them separately because the themes would fit more in files/../config, wheras the static files are not inherently configuration. Please check if the new changes are what you thought of
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.
Leave it like it is if you are not convinced and see how contributors handle it 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.
I have to say I agree with @bgruening here, a structure like
themes
- climate
- theme.yaml
- images/
- welcome.html
would be more user friendly than needing to add things in two places. I'm not sure how difficult that would be to query though from the Ansible side.
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 pushed a commit, it has now a structure like:
subdomains(or whatever)
- climate
- static
- dist
- images
- ...
- themes
- climate.yml
- static
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.
some comments, the use of the new yaml files will be nice!
tool_sections: # The tool section ids to display, generic tools will always be displayed and the list of these can be found in `defaults/main.yml` | ||
subdomains_themes_conf_path: files/galaxy/themes.yml | ||
subdomains_themes_files: "{{ lookup('fileglob', 'files/galaxy/themes/*.yml', wantlist=True) }}" | ||
subdomains_base_indentation: " " |
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 do not have good feelings about this line
if this role runs after galaxy, I have a very strong preference for instead copying the galaxy_config, adding our variables, and writing that out separately that admins should point their systemd units to. thats kind of gross but way, way, way less error prone than hoping the indentation matches up properly.
if this role runs before galaxy_config, I think you should be able to just ammend the configuration and let the galaxy role serialize it properly. This would be my preference but I guess you need to copy static files (is symlinking an option?)
modifying galaxy.yml in this way will present changes every single playbook run.
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 don't get the first part with the systemd units sorry
path: "{{ subdomains_static_path }}/static-{{ subdomain.name }}" | ||
|
||
- name: Move all contents from static to parent directory | ||
ansible.builtin.command: cp -r {{ subdomains_static_dir }}/. {{ subdomains_static_path }}/static-{{ |
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 reason to not use copy remote_src=true?
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 the '-r' is the problem
`galaxy_subsite_base_css` | `#masthead { background-color: #003399;}` | | ||
`galaxy_subsites` | See below | Subdomain listing and configuration. | ||
`galaxy_subsite_nginx_routes` | complex | Some default nginx routes you can plop in your nginx routes. | ||
See defaults for all variables. Most important variables were explained in the previous section. |
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.
then just delete this section
I have revised my opinion: given that this is now a supported galaxy feature, rather than a hack that we built, and is a core feature of galaxy, we should abandon this role and move the features into the main galaxy role. there we would avoid the problems around modifying the galaxy configuration file. it would be cleaner, it would make it easier for others to adopt, it's directly part of managing Galaxy's configuration. |
Co-authored-by: Helena <hexylena@galaxians.org>
closed in favor of: |
see issues: