-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: TNL-10746 CMS API on omnibus plan #241
Conversation
+ studio_host -> cms_host
Separate file was breaking build. Timeboxed efforts to fix while still keeping cms.yaml as a separate file were not fruitful
Ran into a bit of difficulty finding bug that was preventin CI tests from passing. It proved to be missing leading forward slashes on endpoint routes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I added minor comments. The only reason I am not marking as approved is that I did not do a detailed review of all the endpoints, and you are welcome to get a TNL review/approval for that.
swagger/api.yaml
Outdated
@@ -138,6 +138,68 @@ paths: | |||
requestParameters: | |||
integration.request.path.proxy: "method.request.path.proxy" | |||
|
|||
# CMS API (content ingestion) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I'd document this as something like:
# CMS API (content ingestion) | |
# Authoring Domain API (e.g. content ingestion via CMS) |
Notes:
- I think the main header,
Authoring Domain API
, orAuthoring API
, should match the/authoring
in the URL. - I think the descriptive header in parentheses doesn't need to match and can be whatever you want. I just gave one suggestion that combined the other details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Thanks
scripts/aws/deploy_studio.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[inform] You may have mentioned elsewhere the difficulty of renaming this file, but just noting that it is named deploy_studio.py
, and was not renamed, even though the arg was renamed. This is non-blocking, but just noted in case you want to make this more consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. deploy_studio.py
was necessary with the subdomain-based approach, but is superfluous with the omnibus approach. I'm deleting this file: the former deploy.py
file will do just fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks good enough; let's see it run through gocd
Top-level comment marks section for authoring domain, with ingestion as a sample API belonging to the domain deploy_studio.py not needed with authoring API part of the omnibus API
Per recent TNL agreement, we'll be hosting our CMS API on the existing ominbus plan in the API Gateway
These changes add the CMS API to the omnibus swagger YAML description
Also, what was formerly a "studio_host" command line argument now becomes "cms_host". This will be a breaking change on the GoCD script activated on merge and will need fixing there.
To test:
Currently, the GoCD pipeline creates a new API Gateway deployment with the following routes in the "Open edX" API configuration object: /catalog, /enterprise, /heartbeat, ./oauth2, and /registrar. Post-this merge, GoCD needs to continue running without error, but the set of routes on the "Open edX" API objects (both for stage and prod) should include a new /cms/v1 path