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

Close the gap of acs-engine api and ACS public document #936

Closed
rjtsdl opened this issue Jul 7, 2017 · 4 comments · Fixed by #939 or #951
Closed

Close the gap of acs-engine api and ACS public document #936

rjtsdl opened this issue Jul 7, 2017 · 4 comments · Fixed by #939 or #951
Assignees
Milestone

Comments

@rjtsdl
Copy link
Contributor

rjtsdl commented Jul 7, 2017

Is this a request for help?:
No

Is this an ISSUE or FEATURE REQUEST? (choose one):
ISSUE

I am noticing the api model on acs-engine is not aligned with what published here. For example, acs-engine requires user pass in a valid MasterProfile.Count field, but the document clearly states that is not required, and be default to 1.

Orchestrator and version (e.g. Kubernetes, DC/OS, Swarm)
All

What happened:
Once we rollout acs-engine with acs rp in the next one or two months, broader people will notice the breaking change even with the same api-version, such as 2016-09-30. The behavior will be different.

What you expected to happen:
User shouldn't notice we swapped the backend service. As we still conform the same api specs.

How to reproduce it (as minimally and precisely as possible):
Check 2016-09-30's validate function on MasterProfile.Count, apparently 0 is not allowed. The document allows 0, or empty.

@rjtsdl
Copy link
Contributor Author

rjtsdl commented Jul 7, 2017

@amanohar , thx for lighting me into this issue. IMO, I agree, it is P0.
@seanknox, @anhowe , how do you think?

I am thinking to populate default in the unmarshal function, just like the orchestratorType.
@JiangtianLi did the work.

@amanohar
Copy link
Contributor

amanohar commented Jul 7, 2017

Three approaches are:

  1. Make a breaking change (including the old API versions - though I don't think this is advisable) and make all the previously optional fields mandatory (like it is in the new service).
  2. Remain backward compatible for old API versions and enforce all fields to be mandatory on the new API version.
  3. Make it easy for a customer to standup a cluster by providing a minimal set of inputs aka not make fields like count, vmSize, osType mandatory for the new API versions as well by using default values.

I prefer option 3 but I understand that it will need some work in ACS Engine since the current validation is not ACS RP public API validation compatible.

@rjtsdl
Copy link
Contributor Author

rjtsdl commented Jul 7, 2017

I prefer #3 as well.

So i will need to populate default values for version 2016-03-31, 2016-09-30, and 2017-01-31.
For the latest 2017-07-01, i am not sure do it or not.

How to populate?
As i mentioned, i think Unmarshal is the best place. It it before validation. And we have precedence in manipulate the fields in Unmarshal, like OrchestratorType.

@seanknox
Copy link
Contributor

Keeping this open until #951 is delivered.

@seanknox seanknox added this to the v0.4.0 milestone Jul 10, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
4 participants