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

Support more options for new experiments #1113

Merged
merged 6 commits into from
Apr 6, 2023

Conversation

stephen-soltesz
Copy link
Contributor

@stephen-soltesz stephen-soltesz commented Apr 5, 2023

Previously, the update-schema utility only supported legacy datatypes and ndt sidecars. The command assumed that datasets already existed. And the logic was straight forward but very redundant.

This change adds support to update-schema for creating datasets and sidecar tables for alternate experiments. Previous behavior is still available but no longer run by default. Now, the -standard and -legacy options are independent. We typically do not need to update the legacy tables any longer and the template table iteration is time consuming.

Example usage now includes:

# Make standard supported tables.
update-schema -project mlab-sandbox -standard

# Make all datasets and sidecar tables for new foobar experiments.
update-schema -project mlab-sandbox -experiment foobar -sidecars

# Make a single scamper1 table for the wehe experiment.
update-schema -project mlab-sandbox -experiment wehe -datatype scamper1

This change is Reviewable

@coveralls
Copy link
Collaborator

coveralls commented Apr 5, 2023

Pull Request Test Coverage Report for Build 7460

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 4 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.08%) to 67.233%

Files with Coverage Reduction New Missed Lines %
active/active.go 4 88.54%
Totals Coverage Status
Change from base Build 7444: -0.08%
Covered Lines: 3326
Relevant Lines: 4947

💛 - Coveralls

Copy link
Contributor

@cristinaleonr cristinaleonr left a comment

Choose a reason for hiding this comment

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

I know this PR is not starting it, but my opinion is that this command is doing way too many things.

Ideally, only the main() function would be here, and the rest would be in a separate package with unit tests.

Reviewable status: 0 of 1 approvals obtained (waiting on @stephen-soltesz)


cmd/update-schema/update.go line 190 at r2 (raw file):

		if !ok {
			log.Printf("failed to find %v", table)
			return 1

Should this increase the error count and continue?

Copy link
Contributor Author

@stephen-soltesz stephen-soltesz left a comment

Choose a reason for hiding this comment

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

I completely agree. How about this:

  • IOU - migration of this functionality to etl-gardener repo, plus refactoring logic into main + supporting packages with unit tests.

Reviewable status: 0 of 1 approvals obtained (waiting on @cristinaleonr)


cmd/update-schema/update.go line 190 at r2 (raw file):

Previously, cristinaleonr (Cristina Leon) wrote…

Should this increase the error count and continue?

Done.

Copy link
Contributor

@cristinaleonr cristinaleonr left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! 1 of 1 approvals obtained

@stephen-soltesz stephen-soltesz merged commit 71c6cf6 into main Apr 6, 2023
@stephen-soltesz stephen-soltesz deleted the sandbox-soltesz-update-update-schema branch April 6, 2023 18:36
@stephen-soltesz
Copy link
Contributor Author

Thank you!

@stephen-soltesz
Copy link
Contributor Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants