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 Provider and Flavor enums, populate their default values in ProvisionConfigInstance #163

Merged
merged 2 commits into from
Mar 18, 2022

Conversation

engechas
Copy link
Contributor

@engechas engechas commented Mar 17, 2022

Description

Adds the new Provider and Flavor models for the build system expansion.
#132

Signed-off-by: Chase Engelbrecht [email protected]

Check List

  • New functionality includes testing
    • All unit and integration tests pass
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

from enum import Enum


class Provider(str, Enum):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you feel about calling this something like InfraProvider or ClusterInfraProvider instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's more clear, will change

from enum import Enum


class Flavor(str, Enum):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thoughts on calling this ClusterFlavor instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's more clear, will change

Copy link
Collaborator

@IanHoang IanHoang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@engechas engechas merged commit 0425220 into opensearch-project:main Mar 18, 2022

class ClusterFlavor(str, Enum):
SELF_MANAGED = "self_managed"
MANAGED = "managed"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need two different enums for ClusterFlavor & ClusterInfraProvider? Can you explain what's the use case supporting the need for two enums.

I was thinking of combining them into one. The MANAGED flavor seems too generic for me, similarly LOCAL provider probably doesn't add much value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are used in tandem to define the cluster, i.e.

Local + self managed
AWS + self managed
AWS + managed
GCP + managed

While they could be combined into a single enum, separating them will allow easier parsing of the provision config instance file variables when there is commonality across a ClusterInfraProvider type regardless of the ClusterFlavor or vice versa.

The planned structure for PCI file variables is <ClusterInfraProvider>.<ClusterFlavor>.xyz where a special keyword common can be supplied in place of either to act as a wildcard.

A concrete example of this would look something like

[config]
cluster_infra_provider = aws
...

[variables]
aws.common.security_enabled=true
...

By having ClusterInfraProvider as a standalone enum, it becomes very simple to pick up this variable as relevant to the given scenario since we can match on ClusterInfraProvider == aws

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed offline, I recommended merging the enums into one and having provider and favor attributes to it. However, the current form work well for me too.

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

Successfully merging this pull request may close these issues.

4 participants