Skip to content

Remove support schema flags, only use config file. #2221

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

Merged
merged 11 commits into from
Mar 9, 2020

Conversation

gouthamve
Copy link
Contributor

Also rename the schema config file flag to something sane.

Which issue(s) this PR fixes:
Fixes #2208

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@gouthamve gouthamve force-pushed the nuke-schema-flags branch 3 times, most recently from a71faff to 5ff1cab Compare March 6, 2020 11:09
Copy link
Contributor

@sandeepsukhani sandeepsukhani left a comment

Choose a reason for hiding this comment

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

LGTM with very minor fixes.

@gouthamve gouthamve force-pushed the nuke-schema-flags branch from 5ff1cab to 9e07d1c Compare March 6, 2020 11:18
@gouthamve gouthamve force-pushed the nuke-schema-flags branch 5 times, most recently from af006a3 to 42d465f Compare March 6, 2020 16:27
Copy link
Contributor

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

Good job @gouthamve! Happy you approached integration tests for the first time 😉 I left few comments here and there: can't wait to see this PR merged!

Pay attention that:

  • dynamodb.original-table-name is referenced in table-manager-dep.yaml
  • dynamodb.use-periodic-tables, dynamodb.periodic-table, dynamodb.chunk-table have several references across the doc

@gouthamve gouthamve force-pushed the nuke-schema-flags branch 4 times, most recently from 9ef8d8c to f852ca6 Compare March 9, 2020 10:51
Copy link
Contributor

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

LGTM (I left few minor comments)

f.Var(&cfg.ChunkTablesFrom, "dynamodb.chunk-table.from", "Date after which to write chunks to DynamoDB.")
cfg.ChunkTables.RegisterFlags("dynamodb.chunk-table", "cortex_chunks_", f)
flag.StringVar(&cfg.fileName, "schema-config-file", "", "The path to the schema config file.")
// TODO(gouthamve): Add a metric for this.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we wanna keep this TODO? Is it really useful a metric for this?

gouthamve and others added 10 commits March 9, 2020 15:20
Also rename the schema config file flag to something sane.

Signed-off-by: Goutham Veeramachaneni <[email protected]>
Signed-off-by: Goutham Veeramachaneni <[email protected]>
Signed-off-by: Goutham Veeramachaneni <[email protected]>
Signed-off-by: Goutham Veeramachaneni <[email protected]>
Also rename the schema config file flag to something sane.

Signed-off-by: Goutham Veeramachaneni <[email protected]>
Signed-off-by: Goutham Veeramachaneni <[email protected]>
Signed-off-by: Goutham Veeramachaneni <[email protected]>
Signed-off-by: Marco Pracucci <[email protected]>
Signed-off-by: Marco Pracucci <[email protected]>
@pracucci pracucci force-pushed the nuke-schema-flags branch from dac1ecf to 4bd8a88 Compare March 9, 2020 14:31
@pracucci pracucci merged commit 9cf31fe into cortexproject:master Mar 9, 2020
@bboreham bboreham mentioned this pull request Mar 31, 2020
3 tasks
garo added a commit to garo/cortex that referenced this pull request May 6, 2020
Table manager can automatically add AWS tags to created DynamoDB tables.

Previously this was done using a command line argument like “-dynamodb.index-table.tag=foo=bar” but this moved onto the schema configuration in PR cortexproject#2221

This PR updates this old reference on the cli argument.

Signed-off-by: Juho Mäkinen <[email protected]>
pracucci pushed a commit that referenced this pull request May 11, 2020
Table manager can automatically add AWS tags to created DynamoDB tables.

Previously this was done using a command line argument like “-dynamodb.index-table.tag=foo=bar” but this moved onto the schema configuration in PR #2221

This PR updates this old reference on the cli argument.

Signed-off-by: Juho Mäkinen <[email protected]>
simonswine pushed a commit to grafana/e2e that referenced this pull request Jan 13, 2022
…tex#2221)

* Remove support schema flags, only use config file.

Also rename the schema config file flag to something sane.

Signed-off-by: Goutham Veeramachaneni <[email protected]>

* Update docs to use the schema file.

Signed-off-by: Goutham Veeramachaneni <[email protected]>

* Deprecate not remove the config flag.

Signed-off-by: Goutham Veeramachaneni <[email protected]>

* Make integration tests pass?

Signed-off-by: Goutham Veeramachaneni <[email protected]>

* Remove support schema flags, only use config file.

Also rename the schema config file flag to something sane.

Signed-off-by: Goutham Veeramachaneni <[email protected]>

* Update docs to use the schema file.

Signed-off-by: Goutham Veeramachaneni <[email protected]>

* Deprecate not remove the config flag.

Signed-off-by: Goutham Veeramachaneni <[email protected]>

* Update docs/configuration/schema-config-reference.md

Signed-off-by: Marco Pracucci <[email protected]>

* Fixed schema config doc

Signed-off-by: Marco Pracucci <[email protected]>

* Fixes after rebase

Signed-off-by: Marco Pracucci <[email protected]>

* Remove duplicated entry from CHANGELOG

Signed-off-by: Marco Pracucci <[email protected]>

Co-authored-by: Marco Pracucci <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove the schema config from flags and use only config file
5 participants