Skip to content
This repository has been archived by the owner on Dec 6, 2024. It is now read-only.

fix: Allow onboarding member account in non AppStream supported regions #844

Merged
merged 12 commits into from
Jan 3, 2022

Conversation

nguyen102
Copy link
Contributor

@nguyen102 nguyen102 commented Dec 21, 2021

Issue #, if available:

Description of changes:
If you wanted to deploy SWB with AppStream disabled in a non AppStream supported region, you were unable to do so. You will encounter this error in CloudFormation

Template format error: Unrecognized resource types: [AWS::AppStream::StackFleetAssociation, AWS::AppStream::Stack, AWS::AppStream::Fleet]

This code change allows SWB to be deployed in all regions by removing those AppStream specific resources. The resources will be only removed if AppStream is disabled.

Testing: SWB was able to onboard member accounts in both AppStream supported and non supported regions. The upgrade process from an old onboard-account template to the current template was also tested. The upgrade process was smooth. There was no conflicts when using the SWB UI to upgrade the member account.

Checklist:

  • Have you successfully deployed to an AWS account with your changes?
  • Have you written new tests for your core changes, as applicable?

AS review ticket id:

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@codecov
Copy link

codecov bot commented Dec 21, 2021

Codecov Report

Merging #844 (525117e) into develop (a125088) will increase coverage by 0.22%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #844      +/-   ##
===========================================
+ Coverage    50.91%   51.14%   +0.22%     
===========================================
  Files          286      286              
  Lines        15932    15973      +41     
  Branches      2483     2492       +9     
===========================================
+ Hits          8112     8169      +57     
+ Misses        6863     6849      -14     
+ Partials       957      955       -2     
Impacted Files Coverage Δ
lib/cfn-templates/cfn-template-service.js 96.77% <0.00%> (+82.48%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a125088...525117e. Read the comment docs.

@nguyen102 nguyen102 changed the title Member account for nonappstream fix: Allow onboarding member account in non AppStream supported regions Dec 21, 2021
@nguyen102 nguyen102 marked this pull request as ready for review December 21, 2021 21:12
@nguyen102 nguyen102 requested a review from a team as a code owner December 21, 2021 21:12
@@ -48,6 +54,19 @@ class CfnTemplateService extends Service {

async getTemplate(name) {
const entry = _.find(this.store, ['key', name]);
const isAppStreamEnabled = await this.settings.get(settingKeys.isAppStreamEnabled);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we use settings.getBoolean 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.

updated to use getBoolean

Comment on lines 59 to 64
const doc = yamlParse(entry.value);
delete doc.Resources.AppStreamFleet;
delete doc.Resources.AppStreamStack;
delete doc.Resources.AppStreamStackFleetAssociation;
delete doc.Outputs.AppStreamFleet;
delete doc.Outputs.AppStreamStack;
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than deleting specific Resources was it possible to remove the ones where we have condition as isAppStream? That way any future changes to that template (if any) would be accounted for

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated to remove all resources/outputs with isAppStream or isAppStreamAndCustomDomain condition.

@nguyen102 nguyen102 requested review from jn1119 and SanketD92 January 3, 2022 21:03
@nguyen102 nguyen102 merged commit 93dc465 into awslabs:develop Jan 3, 2022
@nguyen102 nguyen102 deleted the member-account-for-nonappstream branch June 5, 2023 19:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants