HARMONY-1873: Add regression image tag support in service deployment. #628
Conversation
There was a problem hiding this comment.
We should probably discuss the name of the field used to identify the test version to run to see what everyone thinks is best.
I ran into a couple of things while testing:
- If the deployment fails to be saved to the database the response is still a 200 with a body like it was successful
{"tag":"latest","statusLink":"http://localhost:3000/service-deployment/34fa6e05-8a5a-4f11-a0a8-175420343c56"}, but then obviously when you go to the link it says deployment not found. We should return a 500 internal server error in that scenario. - Extra parameters are not being validated - for example:
{"tag": "latest", "foo": "bar"}. It would be nice to reject that request with a message that gives a full list of the valid parameters like we do on our request endpoints.
If we don't address those two things in this PR we should write tickets for them.
| @@ -490,13 +497,15 @@ export async function updateServiceImageTag( | |||
| } | |||
|
|
|||
| const { service } = req.params; | |||
| const { tag } = req.body; | |||
| const { tag, test } = req.body; | |||
There was a problem hiding this comment.
I'm wondering about the name to use for the variable to specify the test version to use. test is probably not enough info and I'm not sure if test suites are always going to be in a docker image so I probably wouldn't put image or tag in the name. Maybe testVersion?
There was a problem hiding this comment.
I used test because that is what stated in the ticket. The variable I ended up using in the response is regressionImageTag. I think I am good with either regressionTestVersion or testVersion based on your suggestion. Going to poll the team.
|
|
||
| const deployment = new ServiceDeployment({ | ||
| deployment_id: deploymentId, | ||
| username: req.user, | ||
| service: service, | ||
| tag: tag, | ||
| regression_image_tag: regressionImageTag, |
There was a problem hiding this comment.
Similar comment here in case Docker is not always used - maybe regression_test_version or test_version?
There was a problem hiding this comment.
Renamed to regression_test_version.
| @@ -0,0 +1,11 @@ | |||
| exports.up = function (knex) { | |||
| return knex.schema.alterTable('service_deployments', (t) => { | |||
| t.string('regression_test_version'); | |||
There was a problem hiding this comment.
nit: maybe use t.string('regression_test_version', 255); to use a varchar(255) here?
Jira Issue ID
HARMONY-1873
Description
Add regression image tag support in service deployment.
Local Test Steps
Verify the db migration can be run up and down without any issue.
Submit a service update request with
regression_test_versionfield to indicate the regression image tag to use for regression test:Then, follow the returned deployment status link to see the deployment status, verify that the specified regression test image tag is returned in response. e.g.
PR Acceptance Checklist