Closed pradyutsarma closed 7 years ago
Thanks for the detailed feedback @gberche-orange. I have provided my comments inline.
>>recommend to integrate the suggested unit tests for the OrgEnrollment model
Yes. I will update the PR with the tests.
>> The PR seems to lack reading default values from environment variables specified at autosleep deployment time
As outlined in the my comments above, this PR only introduces the REST APIs and validates the user input. The usage of the values (and the defaults in their absence) will be done by the poller which will be submitted next which actually does will create (or update) the service instances in the necessary spaces.
Swagger APIs
I would like to provide this in a separate PR. Not really comfortable with one PR handling too many things. Created a story for this https://www.pivotaltracker.com/story/show/136915897. Will submit on priority.
Thanks @pradyutsarma for the additional precision on the PR. Besides other comments in this PR:
The PR seems to lack reading default values from environment variables specified at autosleep deployment time As outlined in the my comments above, this PR only introduces the REST APIs and validates the user input. The usage of the values (and the defaults in their absence) will be done by the poller which will be submitted next which actually does will create (or update) the service instances in the necessary spaces.
Agreed, thanks.
Swagger APIs I would like to provide this in a separate PR. Not really comfortable with one PR handling too many things. Created a story for this https://www.pivotaltracker.com/story/show/136915897. Will submit on priority.
Thanks.
I'm sorry @gberche-orange but the PR got polluted by the code from the rebased commits. I'm hoping you will be able to review the new changes.
@pradyutsarma I might have to squash your commits to remove the unintended merge of the guerkins-related commits. This will have the side effect of having the commit misleading attributed to my self instead of you. If this is of importance to you, you might want to prefer to do the squashing yourself (if needed this resource might provide some background on how to do this).
I'll however try the local merge and see if guerkins commit conflicts, and will report soon.
Thanks @pradyutsarma for this contribution and the exchange over the phone. Finally, no need to squash.
We agreed to review the validation aspect in a different story with:
@gberche-orange Created a story to evaluate this https://www.pivotaltracker.com/story/show/138842963