Skip to content
This repository has been archived by the owner on Jul 27, 2023. It is now read-only.

Add --project-name option to init, add the name to .appsody-config.yaml, and validate it against k8s rules #287

Closed
mtamboli opened this issue Sep 9, 2019 · 11 comments · Fixed by #477
Assignees
Milestone

Comments

@mtamboli
Copy link

mtamboli commented Sep 9, 2019

Describe the bug
If the name of the appsody directory is not what Kubernetes service likes, pod will not come up during deployment. Directory name for the appsody project is used for the service.

AppsodyApplication or KnativeService create ServiceSpec objects that must conform to RFC-1123 naming
https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.15/
appsody init should check that the name conforms
https://github.com/kubernetes/kubernetes/blob/v1.15.3/staging/src/k8s.io/apimachinery/pkg/util/validation/validation.go

To Reproduce
Steps to reproduce the behavior:

  1. mkdir jmp/1-2
  2. cd jmp/1-2
  3. appsody init Kabanero/ava-microprofile
  4. appsody deploy --generate-only
  5. app-deploy.xml file is created and it has the directory name "1-2" in that file.
    Then the application was used with Kabanero pipeline, application pod did not start.
    because of the deployment failure:
 Message:           Service "1-2" is invalid: metadata.name: Invalid value: "1-2": a DNS-1035 label must consist of lower case alphanumeric characters or '-', start with an alphabetic character, and end with an alphanumeric character (e.g. 'my-name',  or 'abc-123', regex used for validation is '[a-z]([-a-z0-9]*[a-z0-9])?')

Expected behavior
I would expect error to be displayed during appsody init if directory name is going to be a problem. If not during appsody init, then definitely during `deploy --generate-only'. Need to catch the error earlier.

Environment Details (please complete the following information):
OpenShift 3.1.1
Kabanero release 0.1.2

If applicable please specify:

  • CLI version:
  • Stack you are using:

Screenshots
If applicable, add screenshots to help explain your problem.

Additional context
Add any other context about the problem here.

@chilanti chilanti added the bug Something isn't working label Sep 10, 2019
@chilanti
Copy link
Contributor

chilanti commented Sep 10, 2019

This needs discussion - @neeraj-laad and @kyle.
I think we have been sort of skirting the problem of special characters in Appsody paths for a while (in some cases we turn underscores into dashes, etc.) - but this is a bigger issue, with the potential for a number of consequences (see also #31).
Also - let's keep in mind that Codewind generates temp directories with funky names (and special chars in them).

@chilanti
Copy link
Contributor

@skoh7645 - we had some discussions around this. We'd like to make the following changes:

  1. Change appsody init to take an optional --project-name parm. That name - of provided - should be:
  • validated against the regex [a-z]([-a-z0-9]*[a-z0-9])? and rejected if it doesn't match
  • saved in the .config.yaml
  • retrieved by getProjectName without further validation/tampering with
  1. If not provided, appsody init would generate it based on the project dir:
  • prepend appsody-
  • append -project
  • unless the project dir already starts with appsody and/or end in project
  • then produce a message that says: your project name is appsody-yada-yada-project

@kylegc - feel free to add/edit... @skoh7645 - let us know if questions. Thanks.

@kylegc
Copy link
Member

kylegc commented Sep 12, 2019

I agree this above is the best approach, I will add our reasoning:

  • No matter what name conversion we come up with, some users are not going to be happy with it. This solution gives them a reasonable default and a way to override it if they'd like.
  • Other tools (like codewind) likely rely on the project name to find the container, etc. The config file will give them a reliable way to read the project name rather than duplicating our directory name conversion rules.
  • This provides more transparency to the user on init so they are not confused when see a converted container/deployment name later.

@mtamboli
Copy link
Author

@chilanti : Your design recommendations make sense to me. My concern is that the name may become really long in case the project name is not specified. Would just prefixing appsody- to the directory name be adequate?

@arthurdm you have any input on this?

@arthurdm
Copy link
Member

sounds good to me. In the default case, would an error be produced if the folder name doesn't match the regex?

@skoh7645
Copy link
Collaborator

skoh7645 commented Sep 13, 2019

@chilanti In case 2 we would still need to check the project dir against the regex and convert it if necessary?

@skoh7645
Copy link
Collaborator

@mtamboli we have to make sure the project name starts with an alphabetic char an ends on an alphanumeric char.
Perhaps we should only append/prepend if project name doesn't start/end on a valid char?

@skoh7645 skoh7645 self-assigned this Sep 13, 2019
@chilanti
Copy link
Contributor

@chilanti In case 2 we would still need to check the project dir against the regex and convert it if necessary?

@skoh7645 - yes.

@chilanti
Copy link
Contributor

@mtamboli we have to make sure the project name starts with an alphabetic char an ends on an alphanumeric char.
Perhaps we should only append/prepend if project name doesn't start/end on a valid char?

Right - @mtamboli - it's K8S that's really fussy with names.
After some discussions, we suggest:

  1. Pre-pend and append only if necessary
  2. Append -app instead of -project
  3. Please avoid double - as in appsody-my-dirname--app (which might occur if the dirname ends in a special character, after conversion)

@chilanti
Copy link
Contributor

More details on the design:

  1. Create a new flag in init called --project-name. If not set, default to using the dir name
  2. appsody init saves the project name (either set or generated) in .appsody-config.yaml as key/value project-name: <project name>
  3. getProjectName just gets that value.

@kylegc kylegc changed the title appsody CLI should validate the directory name during init/deploy Add --project-name option to init, add the name to .appsody-config.yaml, and validate it against k8s rules Sep 16, 2019
@ebullient
Copy link

"If not set, default to using the dir name" --> "If not set, default to using the sanitized dir name"

Avoid prefix-suffix games.

@neeraj-laad neeraj-laad modified the milestones: Milestone-6, Milestone-7 Sep 18, 2019
@Bazif-Khan Bazif-Khan assigned Bazif-Khan and unassigned skoh7645 Sep 24, 2019
@neeraj-laad neeraj-laad modified the milestones: Milestone-7, Milestone-8 Oct 4, 2019
tnixa pushed a commit to tnixa/appsody that referenced this issue Nov 7, 2019
We have renamed the template published for nodejs-express from 'skaffold' to 'scaffold' so that it uses proper spelling.

This PR updates doc references to match the change.

Signed-off-by: Neeraj Laad <neeraj.laad@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants