Update EdgeCloud_LcM.yaml #188

gunjald · 2024-02-02T10:26:12Z

The proposed PR aims to simplify and establish relationship between some of the parameters like Uri and component and componentSpec. This changes the AppManifest and provide a description of component and how it relates to componentSpec and what is expected from the API user. The API user know the information of Helm chart or docker-compose internal and what all of them needs to be exposed to external access.

gainsley

Thanks for making these changes!

gainsley · 2024-02-02T16:50:40Z

code/API_definitions/EdgeCloud_LcM.yaml

-              description:  Authorization token acces the artifact repository   
+              type: string
+              maxLength: 128
+              description:  Password or personal access token created by developerto acces the app repository


Why was token removed? I would suggest to leave token in, that way the user can specify either username and password, for which the system will use a HTTP Authorization header of type Basic, or a token, for which the system will use a HTTP Authorization header of type Bearer.

The token as separate field is removed and the password field is described to be placeholder for the token as well. For example Github or Dockerhub and few other repositories accept password and token for a given user name. So in that case we may have separate fields with either or relationship as I could think. Also, in that case we will need to provide another field for repo type e.g. HTTPS, ssh Git credentials.

In most cases where the server accepts either a password or an API key (like Github) with a username, the process is the same in both cases for the client. The OP here is the client, so whether the password field contains a password or an API key, the client doesn't care. It only needs to care if it needs to follow a protocol (like docker login to get a token), or send it formatted as a basic HTTP auth header, or bearer HTTP auth header. I'm fine either way, but if you want to overload the password field I suggest you rename it to "credentials", and then add another field "authType", which could be "docker", "HTTP Basic", or "HTTP Bearer".

I think the following looks a good approach "rename it to "credentials", and then add another field "authType", which could be "docker", "HTTP Basic", or "HTTP Bearer"."

Changes done and committed to PR

gainsley · 2024-02-02T16:57:43Z

code/API_definitions/EdgeCloud_LcM.yaml

@@ -404,6 +404,7 @@ components:
      properties:
        name:
          type: string
+          pattern: ^[A-Za-z][A-Za-z0-9_]{7,63}$


Is there a reason to have a minimum number of characters in the name? We have many examples of names less than 7 characters in our system, i.e. "k8sapp", "myapp", "nfsapp", etc. While maybe not the best names, unless there's a real reason for it, I don't think we should require a minimum number of characters for a name.

Though I agree that minimum length of 7 chars is subjective but i think there should be some minimum. we can not allow 0 length names just as an hypothetical example. But we can discuss if there is a better proposal for this case.

I agree with min length 1 to prevent an empty string. Anything else seems rather arbitrary, though.

ok. Lets go with at least one characters.

Changes done and committed to PR

gainsley · 2024-02-02T17:32:16Z

code/API_definitions/EdgeCloud_LcM.yaml

-        repository:       
+        operatingSystem:
+          $ref: '#/components/schemas/OperatingSystem'
+        appRepo:       


With the name "appRepo", this makes it sound like the URL below is the URL for the repository, i.e. docker.io, rather than the URL for the image, i.e. docker.io/hashicorp/httpecho:2.3.1. I suggest to rename the "url" field below to "imagePath", so it is clear the URL is for the image.

I think this is valid. Will provide a change for this.

Changes done and committed to PR

gainsley · 2024-02-02T17:38:16Z

code/API_definitions/EdgeCloud_LcM.yaml

-              description:  Authorization token acces the artifact repository   
+              type: string
+              maxLength: 128
+              description:  Password or personal access token created by developerto acces the app repository


It is important to have a checksum field so that when our platform pulls the image, we can verify that our copy is correct and we have pulled the exact image the user intended.

I suggest adding the field:

checksum: type: string description: MD5 checksum for VM and file-based images, sha256 digest for containers

Agree. Can incorporate this change.

But there is one issue now. For VM the imagePath can point to the actual image file. But for the Helm charts or docker-compose file the name imagePath doesn't convey the intent. Also, as there could be multiple containers in the file there needs to be a list of container image name and associated checksums. Any thoughts?

I agree Helm is tricky because a chart is really a directory with lots of files in it. There's no way to checksum it unless you package it into an archive. But for anything that's a single file (including docker-compose files), the intent is to checksum the file the OP will download. Anything that another program does, like docker parsing the docker-compose and downloading images, or helm parsing the chart files and downloading images, is really under the control of that program and out of our scope (we don't want to overlap with the functionality of docker or helm). So the intent is just to checksum whatever file is pointed to by imagePath. I would suggest that checksum be optional or not applicable for helm charts since it may be a directory structure instead of a single file.

ok. Will add the checksum field as optional one and add the description for its usage

Changes done and committed to PR

gainsley · 2024-02-02T17:39:47Z

code/API_definitions/EdgeCloud_LcM.yaml

-                    interfaceName:
-                      type: string
-                      description:  Interface Name. Required only if application has to be attatched to a network other then default.
+                      description: Port number exposed by the component. OP may generate a dynamic port towards the UCs corresponding to this internal port  and forward the client traffic from dynamic port to containerPort.


Naming and comment here is confusing, the field is called "extPort" (external port?) but the comment refers to it as both "internal port" and "containerPort", the latter of which doesn't make sense for VM based deployments. Also "UC" is not defined anywhere, so I'm not sure what that stands for.

I would suggest to rename the field back to "port", and have the comment be:

Port number exposed by the component. OP may generate a dynamic port towards the component instance which forwards external traffic to the component port.

Agree with the observation. Will provide a change for it.

Changes done and committed to PR

change Uri to imagePath in appRepo as per comment from Jon.

change extPort to port and updated the description as per the comment from Jon

Changing name field length to at least 1 char as per feedback from Jon

As per suggestion from Jon, updated the password and authType field.

As suggested added the checksum field and description for same

gunjald · 2024-02-05T10:15:52Z

I also propose to add a parameter to appRepo to handle Git repositories. Parameter could take input as repoType that could be one value as Git along with other attributes as revision and path to identify the helm chart chart for example.
Any suggestions please..

gainsley · 2024-02-05T17:12:43Z

I also propose to add a parameter to appRepo to handle Git repositories. Parameter could take input as repoType that could be one value as Git along with other attributes as revision and path to identify the helm chart chart for example. Any suggestions please..

I would suggest to take that up as a separate task from this PR, as its pretty complex and would need some discussion on exactly how it would be handled. For example, what would be the expected file names/directory structure/file types? Would the intent to be to pull a specific branch and commit/tag, or would it be more like a Rancher/ArgoCD CI/CD flow where anytime the branch is updated the OP deploys the new version of the app? My guess would be for each deployment type (docker/kubernetes/helm/vm etc) we'd need to detail to the user exactly what directory and file structure the OP expects to read and how the OP would then attempt to deploy it.

gunjald · 2024-02-06T09:40:41Z

I also propose to add a parameter to appRepo to handle Git repositories. Parameter could take input as repoType that could be one value as Git along with other attributes as revision and path to identify the helm chart chart for example. Any suggestions please..

I would suggest to take that up as a separate task from this PR, as its pretty complex and would need some discussion on exactly how it would be handled. For example, what would be the expected file names/directory structure/file types? Would the intent to be to pull a specific branch and commit/tag, or would it be more like a Rancher/ArgoCD CI/CD flow where anytime the branch is updated the OP deploys the new version of the app? My guess would be for each deployment type (docker/kubernetes/helm/vm etc) we'd need to detail to the user exactly what directory and file structure the OP expects to read and how the OP would then attempt to deploy it.

Seems fine and we can take this proposal in future.

nicolacdnll · 2024-02-06T15:41:23Z

code/API_definitions/EdgeCloud_LcM.yaml

-    CpuArchType:
-          type: string
-          enum:
-            - x86_64
-            - arm_64
-          description: CPU Instruction Set Architecture (ISA) E.g., Intel, Arm etc. 


Not sure if we should remove this. We sometimes see container images build for specific arch. I would like to see what others experience is here.

I think technically I am fully aligned with your comment. Only thing in this attempt is to keep the API simple in first revision with the assumption that first revision is for x86_64 and in future revisions these variations can be introduced again.

Clear, Thanks!

gainsley

Thanks for iterating through those changes!

gunjald · 2024-02-15T15:02:22Z

As no further comments i propose to merge this PR. Any comments can be addressed over this version as they come.

sergiofranciscoortiz

Resolved conflicts with main before approving changes as no further comments from others.

gunjald requested review from Kevsy, crissancas, FabrizioMoggio and sergiofranciscoortiz as code owners February 2, 2024 10:26

gainsley reviewed Feb 2, 2024

View reviewed changes

gunjald added 5 commits February 4, 2024 22:42

Update EdgeCloud_LcM.yaml

28534da

change Uri to imagePath in appRepo as per comment from Jon.

Update EdgeCloud_LcM.yaml

f9744cb

change extPort to port and updated the description as per the comment from Jon

Update EdgeCloud_LcM.yaml

cd72851

Changing name field length to at least 1 char as per feedback from Jon

Update EdgeCloud_LcM.yaml

2c6074b

As per suggestion from Jon, updated the password and authType field.

Update EdgeCloud_LcM.yaml

3c86d5c

As suggested added the checksum field and description for same

sergiofranciscoortiz mentioned this pull request Feb 6, 2024

Review of reported errors from local run CAMARA linter to confirm conformance with API Design Guidelines for EdgeCloud_LcM.yaml #192

Closed

nicolacdnll reviewed Feb 6, 2024

View reviewed changes

gainsley approved these changes Feb 6, 2024

View reviewed changes

nicolacdnll approved these changes Feb 16, 2024

View reviewed changes

Merge branch 'main' into gunjald-patch-4

92532ab

sergiofranciscoortiz approved these changes Feb 16, 2024

View reviewed changes

sergiofranciscoortiz merged commit a7107ea into main Feb 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update EdgeCloud_LcM.yaml #188

Update EdgeCloud_LcM.yaml #188

gunjald commented Feb 2, 2024

gainsley left a comment

gainsley Feb 2, 2024

gunjald Feb 3, 2024

gainsley Feb 4, 2024

gunjald Feb 5, 2024

gunjald Feb 6, 2024

gainsley Feb 2, 2024

gunjald Feb 3, 2024

gainsley Feb 4, 2024

gunjald Feb 5, 2024

gunjald Feb 6, 2024

gainsley Feb 2, 2024

gunjald Feb 3, 2024

gunjald Feb 6, 2024

gainsley Feb 2, 2024

gunjald Feb 3, 2024

gunjald Feb 4, 2024

gainsley Feb 4, 2024

gunjald Feb 5, 2024

gunjald Feb 6, 2024

gainsley Feb 2, 2024

gunjald Feb 3, 2024

gunjald Feb 6, 2024

gunjald commented Feb 5, 2024

gainsley commented Feb 5, 2024

gunjald commented Feb 6, 2024

nicolacdnll Feb 6, 2024

gunjald Feb 6, 2024

nicolacdnll Feb 6, 2024

gainsley left a comment

gunjald commented Feb 15, 2024

sergiofranciscoortiz left a comment

Update EdgeCloud_LcM.yaml #188

Update EdgeCloud_LcM.yaml #188

Conversation

gunjald commented Feb 2, 2024

gainsley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gunjald commented Feb 5, 2024

gainsley commented Feb 5, 2024

gunjald commented Feb 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gainsley left a comment

Choose a reason for hiding this comment

gunjald commented Feb 15, 2024

sergiofranciscoortiz left a comment

Choose a reason for hiding this comment