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

feat(locations): location support #1810

Merged
merged 5 commits into from
Sep 26, 2021

Conversation

ml019
Copy link
Contributor

@ml019 ml019 commented Sep 17, 2021

Intent of Change

  • New feature (non-breaking change which adds functionality)

Description

Add support for Locations on ResourceGroups as a means to determine the placement of resources.

Motivation and Context

Part of ADR0007 implementation.

When determining placements, the resource group id is used as the Location to find the corresponding placement information,
so this means the ids of all resource groups need to be known BEFORE location information is checked.

As a result, when adding a component, any possible resource groups that an implementation might want to use must be
defined. This means that a provider implementer cannot add additional resource groups not already defined when the component was defined.

The existing placement profile approach has been left in place but if present, locations take precedence.

As locations are link based, their introduction increases the chances of creating loops. Code to detect loops has thus
been added when getting Occurrences.

How Has This Been Tested?

Local template generation

Related Changes

Prerequisite PRs:

Dependent PRs:

  • None

Consumer Actions:

  • None

@ml019 ml019 requested a review from a team September 17, 2021 03:37
@ml019 ml019 self-assigned this Sep 17, 2021
@ml019 ml019 marked this pull request as ready for review September 17, 2021 08:51
Copy link
Contributor

@roleyfoley roleyfoley left a comment

Choose a reason for hiding this comment

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

Looks like this might need a rebase? or is it just that things have moved around?

providers/shared/flows/components/flow.ftl Outdated Show resolved Hide resolved
providers/shared/flows/components/flow.ftl Outdated Show resolved Hide resolved
providers/shared/flows/components/flow.ftl Outdated Show resolved Hide resolved
providers/shared/flows/components/flow.ftl Outdated Show resolved Hide resolved
providers/shared/flows/components/flow.ftl Outdated Show resolved Hide resolved
@ml019 ml019 force-pushed the feat-location-based-placement branch from cbd01cf to a5fa586 Compare September 24, 2021 01:22
Add support for Locations on ResourceGroups as a means to determine
the placement of resources.

When determining placements, the resource group id is used
as the Location to find the corresponding placement information,
so this means the ids of all resource groups need to be known
BEFORE location information is checked.

As a result, when adding a component, any possible
resource groups that an implementation might want to use must be
defined. This means that a provider implementer cannot add
additional resource groups not already defined when the component
was defined.

The existing placement profile approach has been left in place
but if present, locations take precedence.

As locations are link based, their introduction increases the
chances of creating loops. Code to detect loops has thus
been added when getting Occurrences.
Include more information about locations in the occurrence structure
to assist debugging configuration issues.

Also move the point at which component configuration for a provider
is loaded so provider specific checks on the provided location values
are applied.
Add a helper function to decouple what information in an occurrence
gets logged in log messages from where the information is needed.

The size of the occurrence structure means it does clutter logs if
provided in full, and the need for the full set of data is rare.

By decoupling the two, additional flexibility around what gets logged
can be added without having to revisit all the places where occurrence
information is included in logs.
Separate the namesapce used for locations from that used for deployment.

Deployment attributes only need to be on deployable components, whereas
Locations apply to the processing of all components.
@ml019 ml019 force-pushed the feat-location-based-placement branch from a5fa586 to 90ac573 Compare September 26, 2021 02:16
@ml019
Copy link
Contributor Author

ml019 commented Sep 26, 2021

@roleyfoley retested with placement: with no issues. Ready for review thanks.

@roleyfoley roleyfoley merged commit 9becc63 into hamlet-io:master Sep 26, 2021
@ml019 ml019 deleted the feat-location-based-placement branch September 26, 2021 11:42
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.

None yet

2 participants