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(quicksight): QuickSight resources and import functionality #23980

Closed
wants to merge 21 commits into from
Closed

feat(quicksight): QuickSight resources and import functionality #23980

wants to merge 21 commits into from

Conversation

Disciple153
Copy link

@Disciple153 Disciple153 commented Feb 3, 2023

Developing QuickSight resources with CDK is currently unnecessarily complicated due to the fact the there is no way to create new QuickSight Analyses, Dashboards, Templates, or Themes without manually gathering data about existing resources and loading them into the QuickSight CfnResources. This PR solves this complexity by allowing QuickSight Resources to be imported and then used as dependencies. This is easier than the current solution because when a CDK QuickSight Resource is used as a dependency, complex objects are automatically mapped from the SDK objects to their CDK/CFN counterparts. This significantly reduces the amount of code that developers need to write to create QuickSight resources.

Addresses #19212

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

@gitpod-io
Copy link

gitpod-io bot commented Feb 3, 2023

@github-actions github-actions bot added bug This issue is a bug. p2 labels Feb 3, 2023
@aws-cdk-automation aws-cdk-automation requested a review from a team February 3, 2023 01:00
@github-actions github-actions bot added the beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK label Feb 3, 2023
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

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

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

@Disciple153 Disciple153 changed the title feat(AWS::QuickSight::*): quicksight resources and import functionality feat(quicksight): quicksight resources and import functionality Feb 3, 2023
@Disciple153 Disciple153 changed the title feat(quicksight): quicksight resources and import functionality feat(quicksight): QuickSight resources and import functionality Feb 3, 2023
@padaszewski
Copy link

+1!

@Disciple153
Copy link
Author

Disciple153 commented Feb 9, 2023

I am currently getting an error in my integ test where the dummy value (undefined) is being returned, where an error should be thrown by my context provider instead. I suspect that my ContextProviders are not set up correctly, and that the default ContextProvider is being used instead, but I'm not sure how to correctly configure my ContextProviders. Any help solving this problem would be greatly appreciated.

Relevant files:

Error:

/home/eppercan/documents/opensource/aws-cdk/packages/@aws-cdk/aws-quicksight/lib/template.js:98
                        throw Error(`No Template found in account ${contextProps.account} and region ${contextProps.region} with id ${contextProps.templateId}`);
                              ^

Error: No Template found in account 732958832353 and region us-west-2 with id test-template
    at Import.get template [as template] (/home/eppercan/documents/opensource/aws-cdk/packages/@aws-cdk/aws-quicksight/lib/template.js:98:31)
    at Import.get templateArn [as templateArn] (/home/eppercan/documents/opensource/aws-cdk/packages/@aws-cdk/aws-quicksight/lib/template.js:157:29)
    at new Analysis (/home/eppercan/documents/opensource/aws-cdk/packages/@aws-cdk/aws-quicksight/lib/analysis.js:40:53)
    at main (/home/eppercan/documents/opensource/aws-cdk/packages/@aws-cdk/aws-quicksight/test/integ.quicksight.js:184:16)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)

(The above error should have been thrown from the TemplateContextProviderPlugin, not from Template.fromId())

@aws-cdk-automation aws-cdk-automation added pr/needs-cli-test-run This PR needs CLI tests run against it. and removed pr/needs-cli-test-run This PR needs CLI tests run against it. labels Feb 13, 2023
@aws-cdk-automation aws-cdk-automation added pr/needs-cli-test-run This PR needs CLI tests run against it. and removed pr/needs-cli-test-run This PR needs CLI tests run against it. labels Feb 16, 2023
@TheRealAmazonKendra
Copy link
Contributor

The bots are out of control... I did this. Putting up a fix ASAP.

@TheRealAmazonKendra
Copy link
Contributor

Manually cancelled the linter for now.

@TheRealAmazonKendra
Copy link
Contributor

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Feb 16, 2023

update

✅ Branch has been successfully updated

@TheRealAmazonKendra
Copy link
Contributor

Sorry for the massive spam on this. I just pushed a fix so I'll watch and make sure it doesn't start looping through the automations again.

@TheRealAmazonKendra
Copy link
Contributor

Looks like I got it fixed.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 79cd86d
  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@Disciple153 Disciple153 closed this by deleting the head repository Feb 23, 2023
@moltar
Copy link
Contributor

moltar commented Feb 23, 2023

What happened @Disciple153? I was looking forward to this PR! :)

@Disciple153
Copy link
Author

Disciple153 commented Feb 23, 2023

@moltar

What happened @Disciple153? I was looking forward to this PR! :)

It's still coming, but after talking to the CDK team, it was decided that this construct will start as a self-published resource.

I will let you know when it becomes available.

Thank you for your interest!

@moltar
Copy link
Contributor

moltar commented Feb 23, 2023

@Disciple153 Please share your repo, I am interested in collaborating. This repo is too noisy and too complicated to foster drive-by collaboration. :)

@aws-cdk-automation
Copy link
Collaborator

The pull request linter fails with the following errors:

❌ Features must contain a change to a README file.
❌ Features must contain a change to an integration test file and the resulting snapshot.
❌ CLI code has changed. A maintainer must run the code through the testing pipeline (git fetch origin pull/23980/head && git push -f origin FETCH_HEAD:test-main-pipeline), then add the 'pr-linter/cli-integ-tested' label when the pipeline succeeds.

PRs must pass status checks before we can provide a meaningful review.

If you would like to request an exemption from the status checks or clarification on feedback, please leave a comment on this PR containing Exemption Request and/or Clarification Request.

@OperationalFallacy
Copy link

OperationalFallacy commented May 20, 2023

We experienced a similar issue. With the frequent changes that data engineers want to make in the Quicksight UI, it's challenging to define Quicksight resources directly from the code.

We addressed this with a LTD (load, transform, deploy) method. We crafted functions such as prepareDataSet which accept a dataset definition and modify it to align with CfnDataSetProps.

For instance, the snippet below uses object-mapper library to map and perform simple transformations like updating account IDs and environments.

const relationalTablePropertyMap = {
  "RelationalTable.Name": "relationalTable.name",
  "RelationalTable.DataSourceArn": {
    key: "relationalTable.dataSourceArn",
    transform: function (value: string) {
      return replaceAccountAndEnv(value, awsAccount, environment);
    },
  },
  "RelationalTable.InputColumns[].Type": "relationalTable.inputColumns[].type",
  "RelationalTable.InputColumns[].Name": "relationalTable.inputColumns[].name",
};

This method proved effective for us in managing an extensive Quicksight setup. Data engineers develop via the Quicksight UI, then use a pipeline for deploying across all environments, including development.

It takes less than hour to add new datasets, mostly waiting for PR review and the pipeline to finish.

@moltar
Copy link
Contributor

moltar commented Jan 16, 2024

It's still coming, but after talking to the CDK team, it was decided that this construct will start as a self-published resource.

@Disciple153 What's the progress on this? Thanks!

@Disciple153
Copy link
Author

@moltar Sorry to disappoint, but unfortunately I haven't been able to work on this in quite a while due to changing priorities from my manager. At this point, I would recommend pulling my PR and using a custom build of CDK. I would really like to finish this at some point, but it will take a lot more work to get this to a quality product with the introduction of the "definition" parameter in the Quicksight SDK, especially since the "definition" parameter is so obnoxious to work with.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK bug This issue is a bug. p2 pr/needs-cli-test-run This PR needs CLI tests run against it.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants