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.

Sign up for GitHub

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

Jump to bottom

[WIP] CCCP API server #656

Closed

navidshaikh wants to merge 20 commits into CentOS:openshift from navidshaikh:apis

Collaborator

navidshaikh commented Oct 15, 2018 •

edited

Loading

This changeset adds flask implementation of CCP readonly APIs.

The API server skeleton is generated by swagger https://swagger.io/
OpenAPI implementation framework using swagger YAML API specifications.

Added Dockerfile for API server at Dockerfiles/ccp-api-server/Dockerfile,
which will containerize the API server and start it.

navidshaikh changed the title ~~CCCP API server~~ [WIP] CCCP API server

navidshaikh force-pushed the apis branch 2 times, most recently from 4ab9d20 to f46fea6 Compare

October 16, 2018 05:56

Collaborator

mohammedzee1000 commented Oct 16, 2018

Looks really cool. We just update the appropriate controllers :)

navidshaikh force-pushed the apis branch 2 times, most recently from 44cd9f2 to 5e81471 Compare

October 18, 2018 10:35

navidshaikh commented

View reviewed changes

ccp/apis/server/ccp_server/models/__init__.py Outdated

+              # from ccp.apis.server.ccp_server.models.all_scanner_logs import AllScannerLogs
+              # from ccp.apis.server.ccp_server.models.app_id_job_id_tag import AppIdJobIdTag
+              # from ccp.apis.server.ccp_server.models.app_id_job_id_tags import AppIdJobIdTags
+              # from ccp.apis.server.ccp_server.models.build_logs import BuildLogs

Collaborator Author

navidshaikh Oct 20, 2018

why are all these imports commented ?
@mohammedzee1000

Collaborator

mohammedzee1000 Oct 20, 2018

They were causing error, once i updated the references. The errors failed once i removed these imports and the server works fine.

Need to still figure out what happened there

navidshaikh commented

View reviewed changes

ccp/apis/server/ccp_server/test/test_projects_controller.py Outdated

+                  ProjectMetadata  # noqa: E501
+              from ccp.apis.server.ccp_server.models.target_file import\
+                  TargetFile  # noqa: E501
+              from ccp.apis.server.ccp_server.test import BaseTestCase

Collaborator Author

navidshaikh Oct 20, 2018

👍 for this change.

navidshaikh commented

View reviewed changes

Dockerfiles/ccp-api-server/Dockerfile Outdated

		@@ -17,10 +17,14 @@ ENV PYTHONPATH="/opt/ccp_api_server"

		COPY . /opt/ccp_api_server

		RUN useradd 1001 && usermod -aG 0 1001 && chown 1001:0 /opt/ccp_api_server && chmod -R g+x /opt/ccp_api_server

Collaborator Author

navidshaikh Oct 20, 2018

chown -R 1001:0 /opt/ccp_api_server

We need to change the permissions to root and all subsequent dirs recursively in that repo, WDYT ?
@mohammedzee1000

Collaborator

mohammedzee1000 Oct 20, 2018

This should not be a problem, but to be safe, yea, i guess it makes sense

navidshaikh commented

View reviewed changes

Dockerfiles/ccp-api-server/Dockerfile Outdated

		@@ -17,10 +17,14 @@ ENV PYTHONPATH="/opt/ccp_api_server"

		COPY . /opt/ccp_api_server

Collaborator Author

navidshaikh Oct 20, 2018

I am thinking, now that /opt/ccp_api_server hosts the complete service code, we should name it as our service repo name or ccp-openshift as we did in slave container.

Collaborator

mohammedzee1000 Oct 20, 2018

It did right from the start

navidshaikh force-pushed the apis branch from 71d89ae to 83e60a8 Compare

October 22, 2018 10:18

navidshaikh force-pushed the apis branch 5 times, most recently from 9c709d6 to 6d37300 Compare

November 1, 2018 06:24

navidshaikh mentioned this pull request

[DoNotMerge]Adds CCCP readonly API specifications #652

Closed

dharmit reviewed

View reviewed changes

Collaborator

dharmit left a comment

This review is only for the swagger.yaml file.

Do we really need to put server code into ccp/apis/v1/server/ccp_server. Is ccp/apis/v1/server not enough? I can't think of another server being written for these APIs.
Ending this review early because of the api endpoints of the format /namespace/{namespace}/app-id/{app_id}/job-id/{job_id}/desired-tag/{desired_tag}. I think we had discussed to go with something like r.c.o/api/v1/{app_id}/{job_id}/{desired_tag}. Not sure why we're having namespace here and why we're making the URLs longer by doing /app-id/{app_id}/job-id/{job_id}/desired-tag/{desired_tag}

ccp/apis/v1/server/ccp_server/swagger/swagger.yaml Outdated

+              basePath: "/api/v1/"
+              tags:
+              - name: "infra"
+                description: "APIs serving infra related information as liveness, help."

Collaborator

dharmit Nov 14, 2018

s/infra/infrastructure

Collaborator

dharmit Nov 14, 2018

Are liveness and help two examples of the kind of information being served? Below sounds more appropriate in that case:

'APIs serving infrastructure related information such as liveness, help.'

ccp/apis/v1/server/ccp_server/swagger/swagger.yaml Outdated

+              - name: "infra"
+                description: "APIs serving infra related information as liveness, help."
+              - name: "meta"
+                description: "APIs serving meta information for projects, namespaces, etc."

Collaborator

dharmit Nov 14, 2018

s/meta/metadata

ccp/apis/v1/server/ccp_server/swagger/swagger.yaml Outdated

+                        schema:
+                          $ref: "#/definitions/Namespaces"
+                    x-swagger-router-controller: "ccp_server.controllers.meta_controller"
+                /namespace/{namespace}/projects:

Collaborator

dharmit Nov 14, 2018

By namespace, do we mean an OpenShift namespace? At the moment, we have only one OpenShift namespace. So how does having this API endpoint help?

Collaborator

dharmit Nov 14, 2018

Also, do we want to have /namespaces and /namespace/{namespace}/projects separate? There's a missing s in namespace in the latter. Or do we mean these two endpoints:

/namespaces
/namespaces/{namespace}/projects

ccp/apis/v1/server/ccp_server/swagger/swagger.yaml Outdated Show resolved Hide resolved

navidshaikh force-pushed the apis branch from 6d37300 to 98294ca Compare

November 15, 2018 14:24

dharmit requested changes

View reviewed changes

Collaborator

dharmit left a comment

Once again, only reviewing the swagger.yaml file because, IIUC, that's the base for rest of the 3,500+ lines of code in this PR.

Can we please use fewer abbreviations like info, meta, etc. for documentation (I can't find a better word for the yaml specification) and use the full words instead?
Do we really need /namespace/{namespace]/? We are having only one namespace in OpenShift at the moment. Yes, we discussed having separate namespaces for projects and their weekly scan builds but that's not in place at the moment. Is it a good idea to have provision for something in the API that's not yet implemented?
Without deciding if we want to fetch details for all builds/weekly-scan build or only for the latest success/failure, we're going to add more logic to the backend or API (wherever we decide to process things.) Is this really required? We could modify the API endpoints and resulting backend functions to just fetch the latest success and failure results, no?

ccp/apis/v1/server/ccp_server/swagger/swagger.yaml Outdated Show resolved Hide resolved

ccp/apis/v1/server/ccp_server/swagger/swagger.yaml Outdated

+                  get:
+                    tags:
+                    - "builds"
+                    summary: "Get all the builds info for given project"

Collaborator

dharmit Nov 15, 2018

"Get information for all builds of the given project" sounds more appropriate.

Collaborator

dharmit Nov 15, 2018

I remember we discussed having only the latest failure and success builds. Does this API endpoint assume otherwise? I think that goes into the def project_builds() function but just raising it so that it's discussed.

ccp/apis/v1/server/ccp_server/swagger/swagger.yaml Outdated

+                    tags:
+                    - "builds"
+                    summary: "Get all the builds info for given project"
+                    description: "Get all the builds number, name and status for given project"

Collaborator

dharmit Nov 15, 2018

"Get build number, name and status of all builds for the given project" sounds appropriate.

ccp/apis/v1/ccp_server/swagger/swagger.yaml Outdated Show resolved Hide resolved

ccp/apis/v1/ccp_server/swagger/swagger.yaml Outdated Show resolved Hide resolved

ccp/apis/v1/ccp_server/swagger/swagger.yaml Show resolved Hide resolved

ccp/apis/v1/ccp_server/swagger/swagger.yaml Outdated Show resolved Hide resolved

ccp/apis/v1/ccp_server/swagger/swagger.yaml Show resolved Hide resolved

ccp/apis/v1/ccp_server/swagger/swagger.yaml Show resolved Hide resolved

ccp/apis/v1/ccp_server/swagger/swagger.yaml Outdated Show resolved Hide resolved

navidshaikh force-pushed the apis branch 3 times, most recently from 99eb0b4 to da0d5ad Compare

November 16, 2018 10:18

bamachrn reviewed

View reviewed changes

ccp/apis/v1/ccp_server/swagger/swagger.yaml Outdated Show resolved Hide resolved

navidshaikh and others added 6 commits

November 21, 2018 13:37


          Adds API server for CCP

32bd54f

 This changeset adds flask implementation of CCP readonly APIs.

 The API server skeleton is generated by swagger <https://swagger.io/>
 OpenAPI implementation framework using swagger YAML API specifications.

 Added Dockerfile for API server at Dockerfiles/ccp-api-server/Dockerfile,
 which will containerize the API server and start it.


          Adds cccp.yml file for building image at registry.centos.org

cbb4ef4


          Updates the WORKDIR path in api-server container

e7e58ee


          Adds backend module structure for wiring up API responses

950d585

 This commit adds backend module structure for wiring up the
 responses for each API response to respective python module file
 in logical API group (infra, meta, projects, builds).

 All of the logical API group are further grouped as per API versioning, i.e. v1 here.


          Adding pythonpath to Dockerfile and __init__ to server.

f3d0d74


          Fixing imports to work with new pythonpath

8339e21

mohammedzee1000 and others added 13 commits

November 21, 2018 13:37


          Fixing api server container to work in randomuid env of OpenShift.

acfae92


          Implements /api/v1/liveness API response

f4fd41e


          Making chown recursive.

5e4d4fc


          Adding oc client and a way to initialise values.

836f6a8


          Fixing up code of models.

3a46525


          Fixing OpenShift version in api server to 3.9 as in current prod.

593d9fa


          Versions complete api server under v1

5290e55


          Makes repository name container-pipeline-service in api-server container

779400f


          Adds OPENSHIFT_VERSION, OPENSHIFT_URL and JENKINS_URL env var

6e6f5c3

 in api-server container, also updates it in env_config.py


          Adds OpenShift template for DeploymentConfig, Service and Route for a…

b6f3330

…pi-server

 Also parameterize the needed values in it.


          Moves the api/v1/server/ccp_server to api/v1/ccp_server/

ddaf1b0

 removes the unnecessary server/ module


          Fixes the typo for project builds API URI swagger.yaml

aa9eccb


          Adds more examples for dataset of models and updates value of examples

70a55bb

 also fixes typo in swagger.yaml

navidshaikh force-pushed the apis branch from 03cad5e to 70a55bb Compare

November 21, 2018 08:20


          Pluralize resources' keywords in API URI schemes

9f165d6

mohammedzee1000 mentioned this pull request

CCCP API Server #668

Merged

Collaborator

bamachrn commented Jan 9, 2019

merged this in #668

bamachrn closed this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet