Create factor table for deploy from settings #400
Conversation
Current coverage is
|
|
I will review it |
|
|
||
| |Factor |1 |2 |3 |4 |5 |6 |7 |Comment| | ||
| |----------------------------|------------------|----------------------------------|----------------|----------------|-------------------------|---------------------|----------------|-------| | ||
| |App name |Empty |Correct value |Max-length |Validation error|Same name app exists | | | | |
There was a problem hiding this comment.
App name has more complexity:
- The field is used to create a pod, RC, SVC and label. The pod has an auto-generated suffix, but SVC and RC get the name of this field. Existence should also check that part e.g. service exists, but RC not.. Think about if there are more factors
- The app name is used as a label (think about if this could create some factors)
- used in the help text which does have some impact on layouting - I would add this as a comment
|
@f-higashi I think we should adapt our style of FBT to OpenSource. I think, the FBT sheet should be less systematic and more interesting to read. Currently, the sheet is somewhat boring to read. So, I would suggest to simplify the sheet. Reduce validation factors and add more business logic factors. We can think about dropping "normal input" and focus about unusual input and situations. This way the tables would get smaller. PTAL |
| |Key(Environment variables) |Empty |Correct value |Max-length |Validation error|Existing key | | | | | ||
| |Value(Environment variables)|Empty |Correct value |Max-length |Validation error| | | | | | ||
| |Action |Deploy |Cancel | | | | | | | | ||
|
|
There was a problem hiding this comment.
I think we should add concurrency explicitly.
|
@cheld But please check other topics and give me your opinion. |
| |Number of pods |Floating point number |Not a number |Max-values | | | | | ||
| |Port |Border cases(1, 65535) |Out of range(<0, >65535) |Floating point number |Not a number |Same port is mapped to different target ports| | | ||
| |Protocol |TCP |UDP | | | | | | ||
| |Exporse service externally|lcoal |vagrant or OpenStack |GCE | | | | |
There was a problem hiding this comment.
Sorry, I forgot...better this way
local | without load-balancer (vagrant) | with load-balancer (GCE)
|
I think it is a good starting point. One small issue. Otherwise LGTM |
f-higashi
force-pushed
the
fb-for-deploy-from-settings
branch
from
f1b3d7b to
b1520e7
Compare
|
Thanks for your review. |
|
Reviewed 1 of 1 files at r3. Comments from the review on Reviewable.io |
|
Merging. |
Create factor table for deploy from settings
I created test factors for 'deploy from settings'.
Now I don't write specific conditions;ex. app name: most 63 characters, matching regex a-z0-9?).
Should I need to write them for all fields?
@cheld