Closed chriscunningham-trivago closed 1 year ago
Thank you for this @chriscunningham-trivago.
This change looks great - thank you for raising the PR :)
One additional note about including the subprocess error from the upstream raised exception - In other occurrences, we've tried to avoid showing exceptions that are generated by external parties, as it could "give away" too much information (which is why the original exception was dumped to stdout). Though, I can obviously see why this information is useful for debugging. I'd be tempted to add a debug flag in the config, or similar, to allow passing this information in the returned API response.
This is incredibly concerning that this has gone broken unnoticed (though our deployed instance is somewhat a little out-of-date).
Whist looking into this, my main concerns/question were:
bin
exists in the repo, it wasn't created on the container./usr/local/bin
- this is configured in the tfswitch command. I'm not 100% sure why, but possibly to avoid nondeterministic location of the result Terraform binary, which then needs to be passed to subsequent subprocess executions of the created Terraform binary.I've started a PR with the changes as they stand: https://gitlab.dockstudios.co.uk/pub/terrareg/-/merge_requests/314
Although, I've had to amend the commit message to include a ticket reference and 'fix:' prefix to conform to semantic release (otherwise a release won't get created and release notes won't be updated :) )
I've also raised https://gitlab.dockstudios.co.uk/pub/terrareg/-/issues/357 to investigate using the main docker file as a source for the docker image that is used to run the automated tests. We previously added a CI step which builds/runs the main docker image, but it only checks that the homepage works, as a small sanity test to ensure it isn't completely broken.
I've added some logic to return the details of original exceptions during module extraction, based on the DEBUG config value: 3c2dbdb58235e6d70b604734e6981a7be9763608 and 82d7197cb69e2c470157e2e07e4cd7121de48284
Let me know what you think - if you don't like it I can remove/amend it :)
The commit from this PR was merged in 9af55f35dd1310d009648d5c4ee0e24e35d2c4fb and released in edbb203e1a5ed0bade874bed8dd9dcc8b35c0479 I'm afraid Github isn't showing the PR as merged as I had to modify the commit message with the angular-style commit message and a ticket number (for the release process).
Thank you for this fix :)
tfswitch attempts to install the
terraform
binary to/app/bin/
but this fails because the directory doesn't exist. Add this directory to the Dockerfile.In addition, bubble up the error which is passed to the
TerraformVersionSwitchError
exception, which helped catch this.