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

feature(sct_config): appendable configuration values #9554

Merged
merged 1 commit into from
Dec 24, 2024

Conversation

fruch
Copy link
Contributor

@fruch fruch commented Dec 15, 2024

introduce new option for appending to string or list configuration values

  1. strings: can be appended with adding ++ at the begining of the string:
    export SCT_APPEND_SCYLLA_ARGS="++ --overprovisioned 1"

  2. list: can be appended by adding ++ as the first item of the list
    export SCT_SCYLLA_D_OVERRIDES_FILES='["++", "extra_file/scylla.d/io.conf"]'

Note list would work on every config defiend with str_or_list_or_eval with the expection of config_files, region_name, gce_datacenter and gce_datacenter which can't be appended, and would need to be overriden completly

Ref: #7653

Testing

  • [ ]

PR pre-checks (self review)

  • I added the relevant backport labels
  • I didn't leave commented-out/debugging code

Reminders

  • Add New configuration option and document them (in sdcm/sct_config.py)
  • Add unit tests to cover my changes (under unit-test/ folder)
  • Update the Readme/doc folder relevant to this change (if needed)

@fruch fruch requested review from dimakr and a team December 15, 2024 13:51
@fruch fruch added the test-provision-aws Run provision test on AWS label Dec 15, 2024
@fruch fruch force-pushed the append_sct_options branch from 88a2fa0 to 8338361 Compare December 16, 2024 08:19
dimakr
dimakr previously approved these changes Dec 16, 2024
Copy link
Contributor

@dimakr dimakr left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@soyacz soyacz left a comment

Choose a reason for hiding this comment

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

we should somehow document it.
Maybe configuration_options.md could have few remarks before options table stating that e.g dicts are merged, or str/lists can be appended like that?

@fruch
Copy link
Contributor Author

fruch commented Dec 16, 2024

we should somehow document it. Maybe configuration_options.md could have few remarks before options table stating that e.g dicts are merged, or str/lists can be appended like that?

all strings configuration can be used like that, and everything with type=str_or_list_or_eval

the configuration_options.md need rework anyhow, it's not really usable nowdays (after github redo their UI)

@fruch
Copy link
Contributor Author

fruch commented Dec 16, 2024

we should somehow document it. Maybe configuration_options.md could have few remarks before options table stating that e.g dicts are merged, or str/lists can be appended like that?

all strings configuration can be used like that, and everything with type=str_or_list_or_eval

the configuration_options.md need rework anyhow, it's not really usable nowdays (after github redo their UI)

#9558 should make that doc a bit more usable
and then we can introduce the information like which one is appendable on the docs

soyacz
soyacz previously approved these changes Dec 17, 2024
Copy link
Contributor

@soyacz soyacz left a comment

Choose a reason for hiding this comment

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

Assuming we're going to document it in followup - LGTM

@fruch fruch added backport/2024.2 Need backport to 2024.2 backport/6.2 and removed test-provision-aws Run provision test on AWS labels Dec 17, 2024
@fruch fruch dismissed stale reviews from soyacz and dimakr via 28a1a94 December 17, 2024 22:00
@fruch fruch force-pushed the append_sct_options branch from 8338361 to 28a1a94 Compare December 17, 2024 22:00
@fruch
Copy link
Contributor Author

fruch commented Dec 17, 2024

Assuming we're going to document it in followup - LGTM

now with the docs update, that would specific which one can be appended and which one can not.
not sure it's clear enough

Copy link
Contributor

@soyacz soyacz left a comment

Choose a reason for hiding this comment

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

I don't see the code that verifies 'appendable' - shouldn't it be added and fail fast?

sdcm/sct_config.py Show resolved Hide resolved
sdcm/sct_config.py Show resolved Hide resolved
@fruch fruch force-pushed the append_sct_options branch 4 times, most recently from a41bf58 to 747658d Compare December 18, 2024 21:26
introduce new option for appending to string or list configuration values

1) strings: can be appended with adding `++` at the begining of the string:
   `export SCT_APPEND_SCYLLA_ARGS="++ --overprovisioned 1"`

2) list: can be appended by adding `++` as the first item of the list
   `export SCT_SCYLLA_D_OVERRIDES_FILES='["++", "extra_file/scylla.d/io.conf"]'`

Note list would work on every config defiend with `str_or_list_or_eval`
with the expection of `config_files`, `region_name`, `gce_datacenter` and `gce_datacenter`
which can't be appended, and would need to be overriden completly

Ref: scylladb#7653
@fruch fruch force-pushed the append_sct_options branch from 747658d to e852d25 Compare December 23, 2024 22:32
Copy link
Contributor

@vponomaryov vponomaryov left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@soyacz soyacz left a comment

Choose a reason for hiding this comment

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

LGTM

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

Successfully merging this pull request may close these issues.

5 participants