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

Fixed Supported ctTSO LabMetadata sample Type Assay combo #514

Merged
merged 1 commit into from
Oct 14, 2022

Conversation

victorskl
Copy link
Member

Fixed #508

@victorskl victorskl self-assigned this Oct 13, 2022
@victorskl victorskl added bug Something isn't working fix labels Oct 13, 2022
@victorskl victorskl added this to the Release 2.0 milestone Oct 13, 2022
Copy link
Member

@alexiswl alexiswl left a comment

Choose a reason for hiding this comment

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

Great work!

@reisingerf
Copy link
Member

This will exclude a specific type/assay combination, but the GH issue mentions to exclude all but the known/supported ones?

Why the "If UNSUPPORTED then fail" instead of "if not SUPPORTED then fail"?

Sorry, I may be missing some context here.

@victorskl
Copy link
Member Author

Eh. El. Ok. Think, I got wut you mean.

You mean other samples type and assay combo? I would not consider those yet. That would be pretty broad meta evaluation that might need done elsewhere... I think. May be, at SampleSheet Checker?

I think, I might write the GH issue description a bit vague. My aim is being specific and, fail-safe switch here. Such that this ctTSO annotation combo is well-known and, the most hotspot failure/confusion point. So want to eliminate that.

@reisingerf
Copy link
Member

I know and I agree that this will eliminate this known issue case.

All I am saying is: are we keeping the door open for other cases where we could easily close it?
Just saying that looking at the past, the issue was usually not in being too restrictive, but in being too permissive/inclusive.

In this case here, I don't know.
What's your opinion @alexiswl ?

@victorskl
Copy link
Member Author

All I am saying is: are we keeping the door open for other cases where we could easily close it?

Sure. Happy to close as much as possible, of course. Think, we need to discuss these other cases... yes.

Just saying that looking at the past, the issue was usually not in being too restrictive, but in being too permissive/inclusive.

That's correct. It was and, is still permissive!

Copy link
Member

@reisingerf reisingerf left a comment

Choose a reason for hiding this comment

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

For the current case it looks OK.
(don't what to stall over improvements)

Copy link
Member Author

@victorskl victorskl left a comment

Choose a reason for hiding this comment

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

(Ok, updated GH issue tickie to reflect specific to ctTSO case.)

Baby step, Flo! Baby step. 👶 We will get there, sure. 😉 And... towards OrcaBus! 🐳

@alexiswl
Copy link
Member

I know and I agree that this will eliminate this known issue case.

All I am saying is: are we keeping the door open for other cases where we could easily close it? Just saying that looking at the past, the issue was usually not in being too restrictive, but in being too permissive/inclusive.

In this case here, I don't know. What's your opinion @alexiswl ?

Yeah I think allowlist is probably a better way to control than a blocklist. But probably need to think about this further.

@victorskl victorskl merged commit eb7c0d2 into dev Oct 14, 2022
@victorskl victorskl deleted the fix-labmeta-sampletype-assay-combo branch October 14, 2022 06:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Harmonise ctTSO LabMeta Type and Assay conditions check between workflows
3 participants