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

docs: proposal for Azure locations + SKUs automation #2694

Merged
merged 1 commit into from
Feb 10, 2020

Conversation

mboersma
Copy link
Member

@mboersma mboersma commented Feb 7, 2020

Reason for Change:
Proposes a solution for some AKS Engine data structures needing to be updated.

Issue Fixed:
See #1894

Requirements:

Notes:


## Non-goals

## Proposal: scriptable aks-engine commands
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love that this proposal also standardizes the way we deliver this information to users.


- Requires significant refactoring and implementation effort
- Making API calls during `aks-engine generate` may be unwanted behavior
- Somewhat slower and less reliable than using the data in code
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that the most concrete downside is absorbing runtime API flakes for all aks-engine ARM operations. Removing that contingency is worth the additional "staleness" of the consts that we statically maintain in code, especially if we reduce the stale time by automating much of the process of updating those static consts.

Copy link
Member

@jackfrancis jackfrancis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

ship it

### Example SKUs output

```shell
$ aks-engine get-skus -o human
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice


## Proposal: scriptable aks-engine commands

New developer-only commands in the `aks-engine` binary generate the necessary Go source code. The commands replace the existing python script and extend it to cover the accelerated networking capability list.
Copy link
Contributor

@marosset marosset Feb 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to take an approach like our e2e tests and wrap that in a make target instead of a new command?
If we do that it might be easier to periodically run the job in a devops pipeline or jenkins job to get a signal of when we need to perform updates.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That signal could be that there were diffs between the checked in generated files and newly generated files.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the presence of a diff in the code should be considered the signal that we need a PR. I forgot to put this explicitly in the doc, but 👍.

Copy link
Member

@jackfrancis jackfrancis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@jackfrancis jackfrancis merged commit 048edd3 into Azure:master Feb 10, 2020
@acs-bot
Copy link

acs-bot commented Feb 10, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jackfrancis, mboersma

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [jackfrancis,mboersma]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@mboersma mboersma deleted the automate-skus branch February 10, 2020 20:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants