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

[FEATURE REQUEST] Separate role vars and manifest vars generation during upgrades #1756

Closed
sk4zuzu opened this issue Oct 8, 2020 · 4 comments
Assignees
Milestone

Comments

@sk4zuzu
Copy link
Contributor

sk4zuzu commented Oct 8, 2020

Is your feature request related to a problem? Please describe.
In this PR #1619 a helper mechanism was introduced to make it possible during upgrades to re-use role config docs taken from manifest.

Currenly it's not enabled for all roles. Also currently enabling manifest vars implies generation of default vars as well. This may cause some issues for certain roles during upgrades.

This FR is about removing this implication and separate those two functionalities to gain fine control over them. Also this separation will surely cause that the whole feature will become more attractive to developers as it may have some potential to simplify the upgrade process. 👍😇

Describe the solution you'd like
Split this list https://github.com/epiphany-platform/epiphany/blob/develop/core/src/epicli/cli/engine/ansible/AnsibleVarsGenerator.py#L63 into two. 👍

Describe alternatives you've considered
N/A

Additional context
N/A

@toszo
Copy link
Contributor

toszo commented Jan 11, 2021

Currently, upgrade overwrites role vars (as far as I remember) and this FR will fix it? Do I understand that correctly?

@sk4zuzu
Copy link
Contributor Author

sk4zuzu commented Jan 11, 2021

Yes, upgrade still overwrites role vars with defaults for specific roles, but we've also added an optional file vars/manifest.yml that is placed next to vars/main.yml (not instead of it). This extra file contains settings from the real manifest and is not loaded by default. Later (during upgrade) we can call a role with manifest vars instead of default vars (or just load them to a dictionary) if needed, like this:

- name: load_balancer | Upgrade haproxy service (runc)
  include_role:
    name: haproxy
    vars_from: manifest

Haproxy component is a good example where this is handy as all the important config is provided by the user.

This FR is to improve this mechnism as currently if we want "manifest vars" we generate "default vars' as well. We'd like to separate those two things. 🤔

@sk4zuzu
Copy link
Contributor Author

sk4zuzu commented Jan 12, 2021

I'm sorry I got maybe a little ahead of myself, but the implementation was extremely short and straightforward #1942. I believe this could help with the grooming... 🤭 + new code is much easier to understand! 👍😇

@sk4zuzu sk4zuzu added this to the S20210114 milestone Jan 12, 2021
@sk4zuzu sk4zuzu self-assigned this Jan 12, 2021
@przemyslavic przemyslavic self-assigned this Jan 12, 2021
@mkyc mkyc modified the milestones: S20210114, S20210128 Jan 14, 2021
@przemyslavic
Copy link
Collaborator

przemyslavic commented Jan 19, 2021

Looks good. Roles tested:
epicli apply: repository, image_registry, node_exporter, haproxy
epicli upgrade: repository, image_registry, node_exporter, haproxy

@mkyc mkyc closed this as completed Jan 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants