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

Prevent panic on invalid configuration structure in exec mixin #2953

Merged
merged 6 commits into from
Feb 21, 2024

Conversation

HeavyBR
Copy link
Contributor

@HeavyBR HeavyBR commented Nov 9, 2023

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

  • Did you write tests?
  • Did you write documentation?
  • Did you change porter.yaml or a storage document record? Update the corresponding schema file.
  • If this is your first pull request, please add your name to the bottom of our Contributors list. Thank you for making Porter better! 🙇‍♀️

Reviewer Checklist

  • Comment with /azp run test-porter-release if a magefile or build script was modified
  • Comment with /azp run porter-integration if it's a non-trivial PR

Signed-off-by: Matheus Cumpian <matheus.cumpian@hotmail.com>
Signed-off-by: Matheus Cumpian <matheus.cumpian@hotmail.com>
Signed-off-by: Matheus Cumpian <matheus.cumpian@hotmail.com>
Signed-off-by: Matheus Cumpian <matheus.cumpian@hotmail.com>
@schristoff
Copy link
Member

Thank you for this PR.
One thing I'm seeing is our "MixinDefinition" needs to be set a little more going forward, so that we don't run into issues where we can just get passed in whatever and hit these panics. But that MixinDefinition type also needs to be loose enough to allow users flexibility. (Just noting this down for our future selves)
Overall, this looks great!
All the tests are happy, so let's get this merged. ✨

@schristoff
Copy link
Member

/azp run porter-integration

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@HeavyBR
Copy link
Contributor Author

HeavyBR commented Nov 22, 2023

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?

@schristoff
Copy link
Member

Looks like TestUninstall_DeleteInstallation failed but it looks like it did so in the Setup stage. Unsure if it is the changes here or in CI.
If you'd like, I'd encourage you to quickly run the test and see if it fails locally, but I can also poke at it later today too ✨

@HeavyBR
Copy link
Contributor Author

HeavyBR commented Nov 22, 2023

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 <28318173+schristoff@users.noreply.github.com>
@schristoff
Copy link
Member

@HeavyBR - very late, but you are right, it was CI and it did work on your machine. I'll get this merged soon.

@schristoff schristoff merged commit d72eba0 into getporter:main Feb 21, 2024
36 checks passed
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.

(panic): exec mixin being passed string instead of a map causes panic
2 participants