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

fix empty directive name #140

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

bradNASA
Copy link
Collaborator

@bradNASA bradNASA commented Jun 6, 2024

Theresa's suggested fix for using the directive type also as the directive name when name is empty for plan uploads.

@bradNASA bradNASA temporarily deployed to integration-test-workflow June 6, 2024 00:48 — with GitHub Actions Inactive
@bradNASA bradNASA temporarily deployed to integration-test-workflow June 6, 2024 00:48 — with GitHub Actions Inactive
@bradNASA bradNASA temporarily deployed to integration-test-workflow June 6, 2024 00:48 — with GitHub Actions Inactive
@bradNASA bradNASA temporarily deployed to integration-test-workflow June 6, 2024 00:48 — with GitHub Actions Inactive
@bradNASA bradNASA temporarily deployed to integration-test-workflow June 6, 2024 00:48 — with GitHub Actions Inactive
@bradNASA bradNASA temporarily deployed to integration-test-workflow June 6, 2024 00:48 — with GitHub Actions Inactive
@bradNASA bradNASA temporarily deployed to integration-test-workflow June 6, 2024 00:48 — with GitHub Actions Inactive
@bradNASA bradNASA temporarily deployed to integration-test-workflow June 6, 2024 00:48 — with GitHub Actions Inactive
@joshhaug joshhaug self-requested a review June 6, 2024 00:49
@joshhaug
Copy link
Collaborator

joshhaug commented Jun 6, 2024

Presumably this would mean that a directive with name == "" is not treated the same as a blank field.

@cartermak
Copy link
Member

@joshhaug My thought too. I'd like that we can preserve an empty name, and only a None gets turned into the activity type name?

Copy link
Collaborator

@joshhaug joshhaug left a comment

Choose a reason for hiding this comment

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

Discussed with Brad. If an empty string is specified as a directive name, it can cause issues in the timeline in 2.12.

@joshhaug
Copy link
Collaborator

joshhaug commented Jun 6, 2024

@cartermak just saw your comment but not sure I understand your question. It looks like there is an issue in 2.12 that causes name=="" to prevent activities from appearing on the timelines.

@cartermak
Copy link
Member

Is this a bug or an expected behavior? If this is a bug, and an empty name should generally be acceptable, then I think Aerie-CLI should support it and I expect Aerie would resolve. If an empty name is not allowed, then I might argue for explicit handling of that case on the client side (e.g., report an error) instead of defaulting to a behavior like automatically assigning a name.

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.

3 participants