Skip to content

Remove dktk and bbmri feature flags#283

Merged
timakro merged 14 commits into
developfrom
remove-dktk-bbmri-feature-flags
Oct 10, 2025
Merged

Remove dktk and bbmri feature flags#283
timakro merged 14 commits into
developfrom
remove-dktk-bbmri-feature-flags

Conversation

@timakro

@timakro timakro commented Sep 30, 2025

Copy link
Copy Markdown
Contributor
  • Project is now selected via project metadata field
  • SQL feature is enabled by default
  • We only build one image samply/focus:main (previously we had samply/focus:main-dktk, samply/focus:main-bbmri)

@enola-dkfz enola-dkfz 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.

Get a green light from @lablans, features are his babies

@enola-dkfz
enola-dkfz requested a review from lablans September 30, 2025 09:24
@timakro
timakro force-pushed the remove-dktk-bbmri-feature-flags branch from 26fd99c to 8ca3fd5 Compare September 30, 2025 09:40

@Threated Threated 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.

So previously this was just decided on the feature flag right?
So unknown projects that talked to a dktk focus would have gotten dktk semantics and did not get rejected?
So this, as is, is definitely a breaking change since there are a lot more "projects" that talk to the dktk focus like exliquid and dktk_supervisors (test lens). But I guess adding these is just a matter of adding enum variants and fixing compile errors.

Comment thread src/projects/mod.rs
@timakro

timakro commented Oct 1, 2025

Copy link
Copy Markdown
Contributor Author

So this, as is, is definitely a breaking change since there are a lot more "projects" that talk to the dktk focus like exliquid and dktk_supervisors (test lens).

The enum is only parsed if it is an AST query and many projects (e.g. exliquid) don't send AST. Maybe there are other projects than DKTK that want DKTK's AST->CQL semantics, we could add them to the enum.

Another breaking change is that some projects might not send the project metadata field at all right now. Would need to add PROJECT env var to their spot config.

@timakro
timakro force-pushed the remove-dktk-bbmri-feature-flags branch from 8b9f81e to 9ca29d0 Compare October 1, 2025 07:48

@Threated Threated 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.

r=me for the code. But we should figure out which projects we might need to add before merging this

Comment thread src/cql.rs Outdated

@Threated Threated 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.

Totally fine with merging this to develop but we need to check that all projects using AST have the project metadata configured correctly.

@timakro
timakro merged commit 287134a into develop Oct 10, 2025
15 checks passed
@timakro
timakro deleted the remove-dktk-bbmri-feature-flags branch October 10, 2025 07:26
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.

4 participants