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.

Sign up for GitHub

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: assert condition command timeout parsing #2171

Merged
merged 1 commit into from
Dec 13, 2024
Merged

fix: assert condition command timeout parsing #2171

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion maestro-orchestra-models/src/main/java/maestro/orchestra/Commands.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -372,7 +372,7 @@ data class AssertCommand(

data class AssertConditionCommand(
val condition: Condition,
private val timeout: String? = null,
val timeout: String? = null,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not too sure how changing scope of variable fixes the issue?

Can you write a test for it as well, so that it not breaks again?

Copy link
Contributor

Choose a reason for hiding this comment

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

no sure how this test would look, since it happens only on Robin. I can add a the test there two if you want

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not too sure how changing scope of variable fixes the issue?

Can you write a test for it as well, so that it not breaks again?

private variables don't get filled out by json parsing. If this is being parsed (it seems to be the case), it'll always be null

Copy link
Collaborator

@amanjeetsingh150 amanjeetsingh150 Dec 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I feel I might be missing something, the deserialization of Yaml commands to Maestro commands works through same deserializer here in YamlCommandReader for both Robin and CLI.

If it's working on CLI and not on Robin, it sounds like we must be doing something wrong on Robin. Not sure it's clear from here what.

Copy link
Contributor

@felipeduartea felipeduartea Dec 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

it seams ot me that this is due to some library version that doesn't parse the json of private variables anymore, as the code implementations is the same in both codes. Still figuring out which one is it. Should I proceed with the release or wait till this is figured out?

I think it probably makes sense to release it that way, since the condition variable already is not private, btw do you have context on that? @amanjeetsingh150

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can go ahead with the release no worries!

But since that can be an issue with private variables. I think more importantly let's ensure there are no other private variables in maestro commands that could also face this issue.

On the version thing probably it makes sense to confirm that we are using same version of serialzers for robin for jackson or some dependency is not missing?

Copy link
Contributor

@felipeduartea felipeduartea Dec 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

But since that can be an issue with private variables. I think more importantly let's ensure there are no other private variables in maestro commands that could also face this issue.

Checked this, it seems that this was the last private val that we had on the MaestroCommand

On the version thing probably it makes sense to confirm that we are using same version of serialzers for robin for jackson or some dependency is not missing?

Also think that it's something related to this, will take a look as soon as I free up

override val label: String? = null,
override val optional: Boolean = false,
) : Command {
Expand Down
Loading