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

chore: remove comments from cilium addons spec #1042

Merged
merged 2 commits into from Apr 15, 2019

Conversation

jackfrancis
Copy link
Member

@jackfrancis jackfrancis commented Apr 12, 2019

Reason for Change:

This saves > 25% against the payload of this yaml

Also includes a reference to docker.io/cilium/operator:v1.4 instead of docker.io/cilium/operator:latest as per @skinny

Issue Fixed:

Requirements:

Notes:

This saves > 25% against the payload of this yaml
@acs-bot acs-bot added the size/L label Apr 12, 2019
@jackfrancis
Copy link
Member Author

@skinny FYI

Copy link
Member

@mboersma mboersma left a comment

Choose a reason for hiding this comment

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

lgtm

@CecileRobertMichon
Copy link
Contributor

/lgtm

@acs-bot
Copy link

acs-bot commented Apr 12, 2019

[APPROVALNOTIFIER] This PR is APPROVED

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

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 [CecileRobertMichon,jackfrancis]

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

@codecov
Copy link

codecov bot commented Apr 12, 2019

Codecov Report

Merging #1042 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1042   +/-   ##
=======================================
  Coverage   74.27%   74.27%           
=======================================
  Files         131      131           
  Lines       18246    18246           
=======================================
  Hits        13552    13552           
  Misses       3912     3912           
  Partials      782      782

Copy link
Contributor

@skinny skinny left a comment

Choose a reason for hiding this comment

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

Wouldn’t this make it harder to keep it in sync? Curious about the motivation. The yaml payload size is not the biggest deal right?

@skinny
Copy link
Contributor

skinny commented Apr 12, 2019

Also the operator image tag in the current addon is set to :latest which breaks a bit against the 1.4 addon, but I will make a seperate PR for that after the weekend

@jackfrancis
Copy link
Member Author

The yaml payload size is not the biggest deal right?

Actually it is. As long as we use Azure's cloud-init interface, we are bound to data limits. E.g.:

Deployment failed. Correlation ID: d8046089-f0da-4856-ad31-2c9cd052c35e. {
  "error": {
    "code": "InvalidParameter",
    "message": "Custom data in OSProfile must be in Base64 encoding and with a maximum length of 87380 characters.",
    "target": "customData"
  }
}

@skinny
Copy link
Contributor

skinny commented Apr 15, 2019

Ah well that sort of rules out the chance of keeping this file in tact. While you merge this PR, can you also change the :latest version of the operator in this file (image: docker.io/cilium/operator:latest) to image: docker.io/cilium/operator:v1.4 to keep stuff running now 1.5 is around the corner

@acs-bot acs-bot removed the lgtm label Apr 15, 2019
@acs-bot
Copy link

acs-bot commented Apr 15, 2019

New changes are detected. LGTM label has been removed.

@jackfrancis
Copy link
Member Author

@skinny done

@jackfrancis jackfrancis merged commit bfdd7fc into Azure:master Apr 15, 2019
@jackfrancis jackfrancis deleted the cilium-addons-comments-reduce branch April 15, 2019 20:42
jackfrancis added a commit to jackfrancis/aks-engine that referenced this pull request Apr 23, 2019
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.

None yet

5 participants