-
Notifications
You must be signed in to change notification settings - Fork 212
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
Prevent panic on invalid configuration structure in exec mixin #2953
Prevent panic on invalid configuration structure in exec mixin #2953
Conversation
Signed-off-by: Matheus Cumpian <[email protected]>
Signed-off-by: Matheus Cumpian <[email protected]>
Signed-off-by: Matheus Cumpian <[email protected]>
Signed-off-by: Matheus Cumpian <[email protected]>
Thank you for this PR. |
/azp run porter-integration |
Azure Pipelines successfully started running 1 pipeline(s). |
Hey @schristoff, seems that the integration tests are not passing, I've tried to found what is happening in the Azure Pipelines logs but I couldn't found, also tried to reproduce locally but nothing. Can you help me with that? |
Looks like |
I would't like to bother you with this but I've tried this test on my machine and I have to say "It works on my machine" 🤣 I'll have to ask you to take a look when you have time, maybe I'm not seeing something 😢 |
Signed-off-by: schristoff <[email protected]>
@HeavyBR - very late, but you are right, it was CI and it did work on your machine. I'll get this merged soon. |
What does this change
Modified the
GetDescription()
method to check if the mixin has the correct structure before try to access their fields, with that we prevent panics on invalid mixins and we return a proper error.What issue does it fix
Closes #2946
Notes for the reviewer
I've started a discussion about this in the issue, but I don´t know if is correct to duplicate the error log as it is doing right now, we're logging in the error span and in the end of the cobra command.
Checklist
Reviewer Checklist