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.

Sign up for GitHub

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

Jump to bottom

allow for import of rds instances and proxies #279

Merged
merged 2 commits into from
Feb 22, 2023
Merged

allow for import of rds instances and proxies #279

merged 2 commits into from
Feb 22, 2023

Conversation

jhsinger-klotho
Copy link
Contributor

• Does any part of it require special attention?
• Does it relate to or fix any issue?

allows for the import of rds and proxy instances

import looks similar to below in pulumi config

  klo:rds:
    sequelizeDB:
      dbInstanceIdentifier: sequelizedbc90e5a2
      proxy: sequelizedb-8bfa7fc

modularizing our pulumi code a bit so that we only add our rds instances if necessary.

tested on ts-sequelize

Standard checks

  • Unit tests: Any special considerations? just imports so no
  • Docs: Do we need to update any docs, internal or public? will add once gordons pr is in since he started the imports page
  • Backwards compatibility: Will this break existing apps? If so, what would be the extra work required to keep them working? yes new feature

atorres-klo
atorres-klo reviewed Feb 22, 2023
View reviewed changes
Copy link
Contributor

@atorres-klo atorres-klo left a comment

Choose a reason for hiding this comment

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

Minor comments, rest LGTM

pkg/infra/pulumi_aws/iac/rds.ts
let rds
let proxy

if (rdsImports != undefined && rdsImports[orm] != undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

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

should we also check for VPC here if required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we dont have to, if they want a separate vpc for their workloads i think we should allow it, it would just be on them to peer their vpcs.

If we want to just because were saying its the only use case we support today then i guess we could, im torn on if thats a great idea or not though

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah that's fair, this is fine for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

especially because in the lambda case it could still connect, its mainly just for the ecs and eks case. So i dont think we want that check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah nevermind it still wouldnt be able to connect because of the Security Group

gordon-klotho
gordon-klotho approved these changes Feb 22, 2023
View reviewed changes
pkg/infra/pulumi_aws/deploylib.ts
proxy: string
}

export const kloConfig: pulumi.Config = new pulumi.Config('klo')
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm surprised this needs the type hint

Copy link
Contributor Author

Choose a reason for hiding this comment

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

doesnt need it, i was trying to get some other things working with config and must have just left it

pkg/infra/pulumi_aws/deploylib.ts Outdated
@@ -62,7 +62,12 @@ export interface TopologyEdgeData {
target: string
}

export const kloConfig = new pulumi.Config('klo')
export interface RdsImport {
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels like it belongs in rds.ts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah it does let me move it

pkg/infra/pulumi_aws/deploylib.ts
@@ -1003,174 +1008,6 @@ export class CloudCCLib {
)
}

public setupRDS(orm: string, args: Partial<aws.rds.InstanceArgs>) {
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO it would have been nice to make the changes and the move separately so it was easier to tell what changed

@github-actions
Copy link

Package Line Rate Health
github.com/klothoplatform/klotho/pkg/analytics 3% ❌
github.com/klothoplatform/klotho/pkg/annotation 23% ❌
github.com/klothoplatform/klotho/pkg/cli 4% ❌
github.com/klothoplatform/klotho/pkg/core 21% ❌
github.com/klothoplatform/klotho/pkg/env_var 82% ✔
github.com/klothoplatform/klotho/pkg/exec_unit 54% ➖
github.com/klothoplatform/klotho/pkg/infra/kubernetes 59% ➖
github.com/klothoplatform/klotho/pkg/infra/kubernetes/helm 39% ❌
github.com/klothoplatform/klotho/pkg/input 72% ➖
github.com/klothoplatform/klotho/pkg/lang 38% ❌
github.com/klothoplatform/klotho/pkg/lang/dockerfile 0% ❌
github.com/klothoplatform/klotho/pkg/lang/golang 36% ❌
github.com/klothoplatform/klotho/pkg/lang/javascript 48% ❌
github.com/klothoplatform/klotho/pkg/lang/python 63% ➖
github.com/klothoplatform/klotho/pkg/lang/yaml 0% ❌
github.com/klothoplatform/klotho/pkg/logging 23% ❌
github.com/klothoplatform/klotho/pkg/multierr 95% ✔
github.com/klothoplatform/klotho/pkg/provider/aws 49% ❌
github.com/klothoplatform/klotho/pkg/provider/aws/resources 70% ➖
github.com/klothoplatform/klotho/pkg/runtime 78% ✔
github.com/klothoplatform/klotho/pkg/static_unit 33% ❌
github.com/klothoplatform/klotho/pkg/updater 30% ❌
github.com/klothoplatform/klotho/pkg/validation 74% ➖
github.com/klothoplatform/klotho/pkg/yaml_util 79% ✔
Summary 43% (4253 / 9936) ❌

@jhsinger-klotho jhsinger-klotho merged commit 80df957 into main Feb 22, 2023
@jhsinger-klotho jhsinger-klotho deleted the rds_import branch February 22, 2023 22:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@atorres-klo atorres-klo atorres-klo left review comments

@gordon-klotho gordon-klotho gordon-klotho approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jhsinger-klotho @gordon-klotho @atorres-klo