search

extdn / github-actions-m2

137 stars 46 forks source link

Allow usage of static encryption key #114

Open Skullsneeze opened 7 months ago

Skullsneeze commented 7 months ago

Magento allows you to specify an encryption key during the application installation. This PR aims to provide this same ability during the setup.

The reason behind this is that a client has a specific test case that relies on data generated using an encryption key. This data is partially stored (statically) in an external service. To ensure the correct outcome I need to be able to specify the encryption key so the resulting encrypted value is not random.

fooman commented 7 months ago

Thanks for your pull request. Have you tried changing this with the pre/post install script already? Another thought is that you should be able to mock the encryption key value used in the decoder for your data in the individual integration test itself.

Skullsneeze commented 7 months ago

Hi @fooman thanks for the quick response.

My initial thought was actually to set this in a post install script using the setup:config:set but that causes issues with serialized values.

I might be able to mock the decryptor, and will give this a go, but I figured that since this is an official setup:install argument it wouldn't hurt adding support for it either way 😊

jissereitsma commented 6 months ago

I agree with @Skullsneeze - the PR is clean and does not seem to break anything, but just add things. And when it is handy, why not? @fooman ?

Skullsneeze commented 6 months ago

FYI, I found that mocking and encryption are quite tricky (at least in the setup we're running). Alternatively I tried adjusting the config, but that is something which is protected by Magento's own integration test framework as it compares the config before and after each test is ran.

I ended up slightly restructuring my code and using Mockery to create a partial mock for one of the classes I was testing. I then created a specific method that would run the validation which uses the encryption and simply let it return true when called on the partial mock.

While it works I do think being able to test with the actual encryption key would be better so I do still think this PR adds value in that sense, but just letting you know that the pressure from my end is currently off ;)

fooman commented 6 months ago

Good stuff @Skullsneeze

And when it is handy, why not? @fooman ?

@jissereitsma happy for it to get merged, just wanted to make sure we don't add more and more options with things that could already be achieved with the existing options