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

feat: Supports PRIVATE_LINK networking type in mongodbatlas_stream_connection resource and data sources #2940

Open
wants to merge 12 commits into
base: CLOUDP-288973-stream-privatelink
Choose a base branch
from
Open

feat: Supports PRIVATE_LINK networking type in mongodbatlas_stream_connection resource and data sources #2940

wants to merge 12 commits into from

Conversation

oarbusi
Copy link
Collaborator

@oarbusi oarbusi commented Jan 7, 2025 •
edited
Loading

Description

Adds support for PRIVATE_LINK networking type in mongodbatlas_stream_connection resource and data sources

Link to any related issue(s): CLOUDP-292668

Type of change:

  • Bug fix (non-breaking change which fixes an issue). Please, add the "bug" label to the PR.
  • New feature (non-breaking change which adds functionality). Please, add the "enhancement" label to the PR. A migration guide must be created or updated if the new feature will go in a major version.
  • Breaking change (fix or feature that would cause existing functionality to not work as expected). Please, add the "breaking change" label to the PR. A migration guide must be created or updated.
  • This change requires a documentation update
  • Documentation fix/enhancement

Required Checklist:

  • I have signed the MongoDB CLA
  • I have read the contributing guides
  • I have checked that this change does not generate any credentials and that they are NOT accidentally logged anywhere.
  • I have added tests that prove my fix is effective or that my feature works per HashiCorp requirements
  • I have added any necessary documentation (if appropriate)
  • I have run make fmt and formatted my code
  • If changes include deprecations or removals I have added appropriate changelog entries.
  • If changes include removal or addition of 3rd party GitHub actions, I updated our internal document. Reach out to the APIx Integration slack channel to get access to the internal document.

Further comments

@github-actions GitHub Actions
Copy link
Contributor

This PR has gone 7 days without any activity and meets the project’s definition of "stale". This will be auto-closed if there is no new activity over the next 7 days. If the issue is still relevant and active, you can simply comment with a "bump" to keep it open, or add the label "not_stale". Thanks for keeping our repository healthy!

@github-actions github-actions bot added the stale label Jan 13, 2025
@oarbusi oarbusi added not_stale Not stale issue or PR and removed stale labels Jan 13, 2025
@oarbusi oarbusi marked this pull request as ready for review January 14, 2025 14:50
@oarbusi oarbusi requested review from a team as code owners January 14, 2025 14:50
@github-actions GitHub Actions
Copy link
Contributor

APIx bot: a message has been sent to Docs Slack channel

EspenAlbert
EspenAlbert reviewed Jan 14, 2025
View reviewed changes
internal/service/streamconnection/model_stream_connection.go
@@ -128,9 +129,18 @@ func NewTFStreamConnection(ctx context.Context, projID, instanceName string, cur

connectionModel.Networking = types.ObjectNull(NetworkingObjectType.AttrTypes)
if apiResp.Networking != nil {
connectionID := types.StringNull()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not use the apiResp.Networking?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

After testing this I realized that the API does not return the ConnectionID, this will be fixed in https://jira.mongodb.org/browse/CLOUDP-294715 and meanwhile I have implemented this to be able to test and use the feature

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This extra implementation will be removed once connectionID is returned, and as you said in the other comment, this affects the import which makes the user experience lack a certain feature. Will push for this to be fixed soon

marcosuma
marcosuma reviewed Jan 14, 2025
View reviewed changes
docs/data-sources/stream_connection.md
@@ -53,8 +53,8 @@ If `type` is of value `Kafka` the following additional attributes are defined:
* `access` - Information about the networking access. See [access](#access).

### Access
* `name` - Id of the vpc peer when the type is `VPC`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

what about name?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

attribute exists in the API and SDK but is not a used attribute. It was created some time ago but it won't be used going forward https://jira.mongodb.org/browse/CLOUDP-294716 . When we first implemented the networking attribute for VPC peering, this attribute existed but was later removed. We removed it from the code but remained in the docs

AgustinBettati
AgustinBettati reviewed Jan 15, 2025
View reviewed changes
internal/service/streamprivatelinkendpoint/resource_migration_test.go Outdated
Comment on lines 11 to 12
acc.SkipTestForCI(t)
mig.SkipIfVersionBelow(t, "1.25.0") // when resource 1st released
Copy link
Member

Choose a reason for hiding this comment

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

Instead of skipping would recommend to adjust the skip version to 1.26.0 so we dont forget to include this test once release is done

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

After discussing with upstream team, we will not run tests for stream private link endpoint in the CI for now. There are ongoing discussions to get credits on Confluent Cloud, but until then we won't run the tests in the CI to avoid unnecessary costs from confluent

Copy link
Member

Choose a reason for hiding this comment

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

thanks for clarifying, always good to capture with a comment for future reference

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1 for the comment!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AgustinBettati AgustinBettati AgustinBettati left review comments

@marcosuma marcosuma marcosuma left review comments

@EspenAlbert EspenAlbert EspenAlbert left review comments

@pierwill pierwill pierwill approved these changes

@lantoli lantoli lantoli approved these changes

Assignees
No one assigned
Labels
enhancement not_stale Not stale issue or PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@oarbusi @lantoli @EspenAlbert @marcosuma @pierwill @AgustinBettati