Fix bug with manual type alias #13671
Conversation
github-actions
bot
added
the
component:awx_collection
| # Alias for manual projects | ||
| if module.params.get('scm_type') == "manual": | ||
| module.params['scm_type'] = '' | ||
|
|
||
| # Extract our parameters | ||
| name = module.params.get('name') | ||
| new_name = module.params.get("new_name") | ||
| copy_from = module.params.get('copy_from') | ||
| scm_type = module.params.get('scm_type') |
There was a problem hiding this comment.
Might as well just pull this line up too and use it above?
| @@ -387,7 +389,8 @@ def main(): | |||
| # this is resolved earlier, so save an API call and don't do it again in the loop above | |||
| project_fields['organization'] = org_id | |||
|
|
|||
| if scm_type == '' and local_path is not None: | |||
| # Respect local_path if scm_type is manual type or not specified | |||
| if scm_type in ('', None) and local_path is not None: | |||
There was a problem hiding this comment.
I am sick and not thinking clearly, but I think this change makes sense. If it's the empty string, it was given by the user as "manual" per above. If it's None, it wasn't given by the user, and (I think) the API will default to "" (manual) if it's a new project.
It looks like in the serializer we already correctly error the case where the user could specify local_path for an SCM type where it doesn't make sense. Still, there are probably some test case scenarios it would be good to add here in the collection just to be explicit. Not 👍-blocking though.
There was a problem hiding this comment.
If it's
None, it wasn't given by the user, and (I think) the API will default to "" (manual) if it's a new project.
This doesn't capture my own motivation for the None case.
If the value is None and there is an existing project, then the existing project could be manual, and thus, allow specifying a local_path. What we really need to avoid is silently dropping the param when we don't know if it's valid or not. If they apply a local_path to a git project, then... don't do that, why would you do that? The API will throw a 400 error message which will have all the error details they need.
|
Let me explain the testing status: It is very challenging to add integration tests for manual projects, because this relies on some corresponding setup with the folders on the server. It is difficult or impossible to automate this in an unopinionated way. Because of that, I wrote a unit test where I have freedom to mock whatever as I see fit. This fails without the fix, and I feel like it's pretty good regression coverage for the fix. |
* Fix bug with manual type alias * Add unit test for creating manual project with path
SUMMARY
While
scm_typelocal variable is changed to''when it gets "manual", it later on gets it frommodule.paramswhich is obviously not aliased. To avoid, we alias earlier. Also, we can't sayscm_typeis not manual when it's not provided (None), so act accordingly.ISSUE TYPE
COMPONENT NAME