ENG-13477: scanner for DocumentDB #4
Conversation
aws/client.go
Outdated
| type docdbCluster struct { | ||
| arn string | ||
| creationDate time.Time | ||
| cluster docdbTypes.DBCluster | ||
| instances []docdbTypes.DBInstance | ||
| tags []string | ||
| } | ||
|
|
||
| func (c *awsClient) getDocumentDBClusters( | ||
| ctx context.Context, | ||
| ) ([]docdbCluster, error) { |
There was a problem hiding this comment.
I believe all we need here are the clusters and the tags directly associated to them. Based on what we briefly discussed today, there's a field (DBClusterMembers) in the cluster object that contains basic information about the cluster's instances (instance name, if its writer or not, etc) and that should be enough information. CC @yoursnerdly @ccampo133 to make sure we agree on that.
If you look at the getRDSClusters, it has a similar structure and all it does is fetch and return a list of clusters. I would suggest this function to follow a similar approach, the only difference I see here is that for DocumentDB we will need to call this other API to fetch the cluster tags. In this case, the docdbCluster could be updated to:
| type docdbCluster struct { | |
| arn string | |
| creationDate time.Time | |
| cluster docdbTypes.DBCluster | |
| instances []docdbTypes.DBInstance | |
| tags []string | |
| } | |
| func (c *awsClient) getDocumentDBClusters( | |
| ctx context.Context, | |
| ) ([]docdbCluster, error) { | |
| type docdbCluster struct { | |
| cluster docdbTypes.DBCluster | |
| tags []string | |
| } | |
| func (c *awsClient) getDocumentDBClusters( | |
| ctx context.Context, | |
| ) ([]docdbCluster, error) { |
There was a problem hiding this comment.
Sure thing. Done.
ricardorey10
force-pushed
the
feature/ENG-13477/docdb
branch
from
58460f0
to
97cd44e
Compare
Make sure unit and integration tests pass Use aws.String() for string pointers Address review comments
There was a problem hiding this comment.
@ricardorey10 just added a few comments below that we need to consider before merging this PR
aws/client.go
Outdated
| DescribeDBInstances( | ||
| ctx context.Context, | ||
| params *docdb.DescribeDBInstancesInput, | ||
| optFns ...func(*docdb.Options), | ||
| ) (*docdb.DescribeDBInstancesOutput, error) |
There was a problem hiding this comment.
Since we're no longer using the DescribeDBInstances this can be removed.
| DescribeDBInstances( | |
| ctx context.Context, | |
| params *docdb.DescribeDBInstancesInput, | |
| optFns ...func(*docdb.Options), | |
| ) (*docdb.DescribeDBInstancesOutput, error) |
There was a problem hiding this comment.
Sure. I thought we had decided to keep all instance tags too, but in order to be more consistent with RDS I agree we should get rid of this.
aws/client.go
Outdated
| clusterARN := clusters[i].DBClusterArn | ||
| var marker *string | ||
| for { | ||
| output, err := c.docdb.DescribeDBInstances( |
There was a problem hiding this comment.
We no longer need to retrieve the cluster instances, since we are not returning it anymore. Same for instances tags. Like I mentioned in my previous comment, I believe just getting the cluster tags here should be enough.
aws/repository.go
Outdated
| Type: scan.RepoTypeDocumentDB, | ||
| CreatedAt: *cluster.cluster.ClusterCreateTime, | ||
| Tags: cluster.tags, | ||
| Properties: cluster.cluster.DBClusterMembers, |
There was a problem hiding this comment.
The Properties should be the whole cluster object instead of just the cluster members. Similar to how its done in newRepositoryFromRDSCluster.
| Properties: cluster.cluster.DBClusterMembers, | |
| Properties: cluster.cluster, |
ricardorey10
force-pushed
the
feature/ENG-13477/docdb
branch
from
97cd44e
to
823d344
Compare
aws/client.go
Outdated
|
|
||
| // Phew, that was a lot of work, but we have all that we wanted: | ||
| // All clusters in the <clusters> variable | ||
| // A map from cluster ARN to instances, in the <instances> variable |
There was a problem hiding this comment.
| // A map from cluster ARN to instances, in the <instances> variable |
Description of the change
This just implements the Scanner logic for DocumentDB repositories.
Type of change
Checklists
Development
Code review
Testing
Unit + integration tests.