Skip to content

refactor: move config props to where they belong - #462

Merged
alexanto merged 3 commits into
mainfrom
refactor/remove-metrics_interval_str_millis
Oct 29, 2026
Merged

alexanto merged 3 commits into
mainfrom
refactor/remove-metrics_interval_str_millis

Conversation

@alexanto

@alexanto alexanto commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

opinion: with an immutable config there's no reason for these separate properties
there are only used at one place each

@alexanto
alexanto added this pull request to stack #463 October 28, 2026 12:54
@alexanto alexanto changed the title chore: remove metrics interval str millis refacrot: move config props to where they belong Oct 28, 2026
@alexanto alexanto changed the title refacrot: move config props to where they belong refactor: move config props to where they belong Oct 28, 2026

@ppicom ppicom left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice! I'd just double check why CodeQL feels like we are going to leak secrets with that log line.

if mode == InstanceAllowType.BLOCK:
raise MultipleInstancesNotAllowedError(msg)
if mode == InstanceAllowType.WARN:
LOGGER.error(msg)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

CodeQL flagged this log line as a potential leak. Can we double check it? I see that we moved redact_to_print_safely to a different location but it's still in use, so no biggie.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

i think CodeQL gives a false positive here, doesn't know that redact_to_print_safely is a sanitizer
and i checked if i just remove the @Property it already fires this warning, so i think it was the decorator that made CodeQL skip it

@gastonfournier gastonfournier moved this from New to In Progress in Issues and PRs Oct 29, 2026
Base automatically changed from refactor/remove-environment-option to main October 29, 2026 07:31
@alexanto
alexanto force-pushed the refactor/remove-metrics_interval_str_millis branch from 4b77c61 to 4ca2468 Compare October 29, 2026 07:32

@chriswk chriswk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good to me

@github-project-automation github-project-automation Bot moved this from In Progress to Approved PRs in Issues and PRs Oct 29, 2026
@alexanto
alexanto merged commit b47f263 into main Oct 29, 2026
8 checks passed
@alexanto
alexanto deleted the refactor/remove-metrics_interval_str_millis branch October 29, 2026 10:41
@github-project-automation github-project-automation Bot moved this from Approved PRs to Done in Issues and PRs Oct 29, 2026
@Elyahou Elyahou mentioned this pull request Oct 29, 2026
8 of 9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants