-
Notifications
You must be signed in to change notification settings - Fork 297
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: assert condition command timeout parsing #2171
Conversation
@@ -372,7 +372,7 @@ data class AssertCommand( | |||
|
|||
data class AssertConditionCommand( | |||
val condition: Condition, | |||
private val timeout: String? = null, | |||
val timeout: String? = null, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
7bed304
to
5e21eb3
Compare
Rebased |
How do you reproduce the problem? |
Locally, I tried (using the release version):
Which times out after 60s, as it should. Is the serialisation problem only a problem for Robin? |
Yeah, this issue only happens in Robin serialization, since the timeout var is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should probably review all the commands to see which ones have private vals and make them public, since this might be leading to other issues we're not seeing yet
Proposed changes
Removing private access modifier from timeout field
Issues fixed
Private modifier on timeout field is preventing the field to be populated on deserialization.