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

Support dynamic resource suffix in Airlock #3243

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

yuvalyaron
Copy link
Collaborator

@yuvalyaron yuvalyaron commented Feb 22, 2023

Resolves #3237

What is being addressed

  • The code in the airlock processor references certain storage accounts by assuming that their names have the workspace short ID as a suffix. Rather than making this assumption, the suffix was changed to be passed in the 'status changed' message.

  • Added Migration to add the unique_identifier_suffix field to all resources. As the changes in Workspace deploy fails because storage account is not unique #2893 expect this field to exist in all resources. The value that the migration is adding is the last 4 digits of the resource as this was the value that we used before the changes.

How is this addressed

  • Added migration to add the unique_identifier_suffix field.
  • Replaced workspace ID with unique_identifier_suffix in airlock processor
  • Fixed unit tests

@yuvalyaron yuvalyaron marked this pull request as ready for review February 22, 2023 17:23
@yuvalyaron yuvalyaron requested a review from eladiw February 22, 2023 17:23
@github-actions
Copy link

github-actions bot commented Feb 22, 2023

Unit Test Results

617 tests   608 ✅  8s ⏱️
  2 suites    2 💤
  2 files      7 ❌

For more details on these failures, see this check.

Results for commit 736233c.

♻️ This comment has been updated with latest results.

@@ -18,7 +18,7 @@ class RequestProperties(BaseModel):
new_status: str
previous_status: Optional[str]
type: str
workspace_id: str
unique_identifier_suffix: str
Copy link
Collaborator

Choose a reason for hiding this comment

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

Across much of this change, isn't the unique_identifier_suffix always the workspace one?
If so, shouldn't we call it like that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

changed

Copy link
Contributor

@eladiw eladiw left a comment

Choose a reason for hiding this comment

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

Lgtm

@tamirkamara tamirkamara marked this pull request as draft March 1, 2023 06:40
@tamirkamara tamirkamara added the blocked Cannot progress at present label Jul 25, 2023
@marrobi
Copy link
Member

marrobi commented Sep 27, 2023

@yuvalyaron @tamirkamara can you recall where this got to? We are hitting some storage account name conflicts on the airlock accounts...

import_approved_storage_name = lower(replace("stalimapp${substr(local.workspace_resource_name_suffix, -8, -1)}", "-", ""))

This needs to include say the tre_id to reduce the risk of a conflict.

@tamirkamara
Copy link
Collaborator

@yuvalyaron @tamirkamara can you recall where this got to? We are hitting some storage account name conflicts on the airlock accounts...

import_approved_storage_name = lower(replace("stalimapp${substr(local.workspace_resource_name_suffix, -8, -1)}", "-", ""))

This needs to include say the tre_id to reduce the risk of a conflict.

This PR depends on another large development we paused for now. It looks for a workspace property that notes the random suffix of resource names like the storage accounts.
So this PR specifically should remain agnostic to what value is chosen for that suffix.

@tim-p-allen tim-p-allen added api Composition Service API airlock and removed blocked Cannot progress at present labels May 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
airlock api Composition Service API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Airlock should support dynamic resource suffix
6 participants