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

Adopt protogetter #5981

Merged
merged 16 commits into from
Nov 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
The table of contents is too big for display.
Diff view
Diff view
  •  
  •  
  •  
233 changes: 119 additions & 114 deletions boilerplate/flyte/golang_support_tools/go.mod

Large diffs are not rendered by default.

898 changes: 263 additions & 635 deletions boilerplate/flyte/golang_support_tools/go.sum

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion boilerplate/flyte/golang_test_targets/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ generate: download_tooling #generate go code

.PHONY: lint
lint: download_tooling #lints the package for common code smells
GL_DEBUG=linters_output,env golangci-lint run $(LINT_FLAGS) --deadline=5m --exclude deprecated -v
GL_DEBUG=linters_output,env golangci-lint run $(LINT_FLAGS) --timeout=5m --exclude deprecated -v

.PHONY: lint-fix
lint-fix: LINT_FLAGS=--fix
Expand Down
16 changes: 4 additions & 12 deletions datacatalog/.golangci.yml
Original file line number Diff line number Diff line change
@@ -1,35 +1,25 @@
# WARNING: THIS FILE IS MANAGED IN THE 'BOILERPLATE' REPO AND COPIED TO OTHER REPOSITORIES.
# ONLY EDIT THIS FILE FROM WITHIN THE 'FLYTEORG/BOILERPLATE' REPOSITORY:
#
# TO OPT OUT OF UPDATES, SEE https://github.com/flyteorg/boilerplate/blob/master/Readme.rst

run:
skip-dirs:
- pkg/client

linters:
disable-all: true
enable:
- deadcode
- errcheck
- gas
- gosec
- gci
- goconst
- goimports
- golint
- gosimple
- govet
- ineffassign
- misspell
- nakedret
- staticcheck
- structcheck
- typecheck
- unconvert
- unparam
- unused
- varcheck

- protogetter
linters-settings:
gci:
custom-order: true
Expand All @@ -38,6 +28,8 @@ linters-settings:
- default
- prefix(github.com/flyteorg)
skip-generated: true
goconst:
ignore-tests: true
issues:
exclude:
- copylocks
6 changes: 3 additions & 3 deletions datacatalog/pkg/manager/impl/artifact_data_store.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,8 @@ type artifactDataStore struct {
}

func (m *artifactDataStore) getDataLocation(ctx context.Context, artifact *datacatalog.Artifact, data *datacatalog.ArtifactData) (storage.DataReference, error) {
dataset := artifact.Dataset
return m.store.ConstructReference(ctx, m.storagePrefix, dataset.Project, dataset.Domain, dataset.Name, dataset.Version, artifact.Id, data.Name, artifactDataFile)
dataset := artifact.GetDataset()
return m.store.ConstructReference(ctx, m.storagePrefix, dataset.GetProject(), dataset.GetDomain(), dataset.GetName(), dataset.GetVersion(), artifact.GetId(), data.GetName(), artifactDataFile)
}

// Store marshalled data in data.pb under the storage prefix
Expand All @@ -37,7 +37,7 @@ func (m *artifactDataStore) PutData(ctx context.Context, artifact *datacatalog.A
if err != nil {
return "", errors.NewDataCatalogErrorf(codes.Internal, "Unable to generate data location %s, err %v", dataLocation.String(), err)
}
err = m.store.WriteProtobuf(ctx, dataLocation, storage.Options{}, data.Value)
err = m.store.WriteProtobuf(ctx, dataLocation, storage.Options{}, data.GetValue())
if err != nil {
return "", errors.NewDataCatalogErrorf(codes.Internal, "Unable to store artifact data in location %s, err %v", dataLocation.String(), err)
}
Expand Down
50 changes: 25 additions & 25 deletions datacatalog/pkg/manager/impl/artifact_manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,16 +58,16 @@
timer := m.systemMetrics.createResponseTime.Start(ctx)
defer timer.Stop()

artifact := request.Artifact
artifact := request.GetArtifact()
err := validators.ValidateArtifact(artifact)
if err != nil {
logger.Warningf(ctx, "Invalid create artifact request %v, err: %v", request, err)
m.systemMetrics.validationErrorCounter.Inc(ctx)
return nil, err
}

ctx = contextutils.WithProjectDomain(ctx, artifact.Dataset.Project, artifact.Dataset.Domain)
datasetKey := transformers.FromDatasetID(artifact.Dataset)
ctx = contextutils.WithProjectDomain(ctx, artifact.GetDataset().GetProject(), artifact.GetDataset().GetDomain())
datasetKey := transformers.FromDatasetID(artifact.GetDataset())

// The dataset must exist for the artifact, let's verify that first
dataset, err := m.repo.DatasetRepo().Get(ctx, datasetKey)
Expand All @@ -80,29 +80,29 @@
// TODO: when adding a tag, need to verify one tag per partition combo
// check that the artifact's partitions are the same partition values of the dataset
datasetPartitionKeys := transformers.FromPartitionKeyModel(dataset.PartitionKeys)
err = validators.ValidatePartitions(datasetPartitionKeys, artifact.Partitions)
err = validators.ValidatePartitions(datasetPartitionKeys, artifact.GetPartitions())
if err != nil {
logger.Warnf(ctx, "Invalid artifact partitions %v, err: %+v", artifact.Partitions, err)
logger.Warnf(ctx, "Invalid artifact partitions %v, err: %+v", artifact.GetPartitions(), err)
m.systemMetrics.createFailureCounter.Inc(ctx)
return nil, err
}

// create Artifact Data offloaded storage files
artifactDataModels := make([]models.ArtifactData, len(request.Artifact.Data))
for i, artifactData := range request.Artifact.Data {
artifactDataModels := make([]models.ArtifactData, len(request.GetArtifact().GetData()))
for i, artifactData := range request.GetArtifact().GetData() {
dataLocation, err := m.artifactStore.PutData(ctx, artifact, artifactData)
if err != nil {
logger.Errorf(ctx, "Failed to store artifact data err: %v", err)
m.systemMetrics.createDataFailureCounter.Inc(ctx)
return nil, err
}

artifactDataModels[i].Name = artifactData.Name
artifactDataModels[i].Name = artifactData.GetName()
artifactDataModels[i].Location = dataLocation.String()
m.systemMetrics.createDataSuccessCounter.Inc(ctx)
}

logger.Debugf(ctx, "Stored %v data for artifact %+v", len(artifactDataModels), artifact.Id)
logger.Debugf(ctx, "Stored %v data for artifact %+v", len(artifactDataModels), artifact.GetId())

artifactModel, err := transformers.CreateArtifactModel(request, artifactDataModels, dataset)
if err != nil {
Expand All @@ -114,7 +114,7 @@
err = m.repo.ArtifactRepo().Create(ctx, artifactModel)
if err != nil {
if errors.IsAlreadyExistsError(err) {
logger.Warnf(ctx, "Artifact already exists key: %+v, err %v", artifact.Id, err)
logger.Warnf(ctx, "Artifact already exists key: %+v, err %v", artifact.GetId(), err)

Check warning on line 117 in datacatalog/pkg/manager/impl/artifact_manager.go

View check run for this annotation

Codecov / codecov/patch

datacatalog/pkg/manager/impl/artifact_manager.go#L117

Added line #L117 was not covered by tests
m.systemMetrics.alreadyExistsCounter.Inc(ctx)
} else {
logger.Errorf(ctx, "Failed to create artifact %v, err: %v", artifactDataModels, err)
Expand All @@ -123,7 +123,7 @@
return nil, err
}

logger.Debugf(ctx, "Successfully created artifact id: %v", artifact.Id)
logger.Debugf(ctx, "Successfully created artifact id: %v", artifact.GetId())

m.systemMetrics.createSuccessCounter.Inc(ctx)
return &datacatalog.CreateArtifactResponse{}, nil
Expand All @@ -141,7 +141,7 @@
return nil, err
}

datasetID := request.Dataset
datasetID := request.GetDataset()

artifactModel, err := m.findArtifact(ctx, datasetID, request)
if err != nil {
Expand All @@ -164,7 +164,7 @@
}
artifact.Data = artifactDataList

logger.Debugf(ctx, "Retrieved artifact dataset %v, id: %v", artifact.Dataset, artifact.Id)
logger.Debugf(ctx, "Retrieved artifact dataset %v, id: %v", artifact.GetDataset(), artifact.GetId())
m.systemMetrics.getSuccessCounter.Inc(ctx)
return &datacatalog.GetArtifactResponse{
Artifact: artifact,
Expand Down Expand Up @@ -249,7 +249,7 @@
}

// Verify the dataset exists before listing artifacts
datasetKey := transformers.FromDatasetID(request.Dataset)
datasetKey := transformers.FromDatasetID(request.GetDataset())
dataset, err := m.repo.DatasetRepo().Get(ctx, datasetKey)
if err != nil {
logger.Warnf(ctx, "Failed to get dataset for listing artifacts %v, err: %v", datasetKey, err)
Expand All @@ -265,7 +265,7 @@
return nil, err
}

err = transformers.ApplyPagination(request.Pagination, &listInput)
err = transformers.ApplyPagination(request.GetPagination(), &listInput)
if err != nil {
logger.Warningf(ctx, "Invalid pagination options in list artifact request %v, err: %v", request, err)
m.systemMetrics.validationErrorCounter.Inc(ctx)
Expand Down Expand Up @@ -311,7 +311,7 @@
// stored data will be overwritten in the underlying blob storage, no longer existing data (based on ArtifactData name)
// will be deleted.
func (m *artifactManager) UpdateArtifact(ctx context.Context, request *datacatalog.UpdateArtifactRequest) (*datacatalog.UpdateArtifactResponse, error) {
ctx = contextutils.WithProjectDomain(ctx, request.Dataset.Project, request.Dataset.Domain)
ctx = contextutils.WithProjectDomain(ctx, request.GetDataset().GetProject(), request.GetDataset().GetDomain())

timer := m.systemMetrics.updateResponseTime.Start(ctx)
defer timer.Stop()
Expand All @@ -333,9 +333,9 @@
}

// artifactModel needs to be updated with new SerializedMetadata
serializedMetadata, err := transformers.SerializedMetadata(request.Metadata)
serializedMetadata, err := transformers.SerializedMetadata(request.GetMetadata())
if err != nil {
logger.Errorf(ctx, "Error in transforming Metadata from request %+v, err %v", request.Metadata, err)
logger.Errorf(ctx, "Error in transforming Metadata from request %+v, err %v", request.GetMetadata(), err)

Check warning on line 338 in datacatalog/pkg/manager/impl/artifact_manager.go

View check run for this annotation

Codecov / codecov/patch

datacatalog/pkg/manager/impl/artifact_manager.go#L338

Added line #L338 was not covered by tests
m.systemMetrics.transformerErrorCounter.Inc(ctx)
m.systemMetrics.updateFailureCounter.Inc(ctx)
return nil, err
Expand All @@ -353,9 +353,9 @@
// overwrite existing artifact data and upload new entries, building a map of artifact data names to remove
// deleted entries from the blob storage after the upload completed
artifactDataNames := make(map[string]struct{})
artifactDataModels := make([]models.ArtifactData, len(request.Data))
for i, artifactData := range request.Data {
artifactDataNames[artifactData.Name] = struct{}{}
artifactDataModels := make([]models.ArtifactData, len(request.GetData()))
for i, artifactData := range request.GetData() {
artifactDataNames[artifactData.GetName()] = struct{}{}

dataLocation, err := m.artifactStore.PutData(ctx, artifact, artifactData)
if err != nil {
Expand All @@ -365,7 +365,7 @@
return nil, err
}

artifactDataModels[i].Name = artifactData.Name
artifactDataModels[i].Name = artifactData.GetName()
artifactDataModels[i].Location = dataLocation.String()
m.systemMetrics.updateDataSuccessCounter.Inc(ctx)
}
Expand All @@ -384,7 +384,7 @@
err = m.repo.ArtifactRepo().Update(ctx, artifactModel)
if err != nil {
if errors.IsDoesNotExistError(err) {
logger.Warnf(ctx, "Artifact does not exist key: %+v, err %v", artifact.Id, err)
logger.Warnf(ctx, "Artifact does not exist key: %+v, err %v", artifact.GetId(), err)

Check warning on line 387 in datacatalog/pkg/manager/impl/artifact_manager.go

View check run for this annotation

Codecov / codecov/patch

datacatalog/pkg/manager/impl/artifact_manager.go#L387

Added line #L387 was not covered by tests
m.systemMetrics.doesNotExistCounter.Inc(ctx)
} else {
logger.Errorf(ctx, "Failed to update artifact %v, err: %v", artifactModel, err)
Expand All @@ -408,11 +408,11 @@
m.systemMetrics.deleteDataSuccessCounter.Inc(ctx)
}

logger.Debugf(ctx, "Successfully updated artifact id: %v", artifact.Id)
logger.Debugf(ctx, "Successfully updated artifact id: %v", artifact.GetId())

m.systemMetrics.updateSuccessCounter.Inc(ctx)
return &datacatalog.UpdateArtifactResponse{
ArtifactId: artifact.Id,
ArtifactId: artifact.GetId(),
}, nil
}

Expand Down
Loading
Loading