Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Position services without annotations. #38

Merged
merged 4 commits into from
Nov 10, 2015
Merged

Position services without annotations. #38

merged 4 commits into from
Nov 10, 2015

Conversation

makyo
Copy link
Contributor

@makyo makyo commented Nov 4, 2015

This is a backwards compatible change to place services which do not have position annotations. It works on the case of a bundle with one/some missing service annotations as well as a bundle missing all annotations, both of which pass bundle proof.

@jujugui
Copy link
Contributor

jujugui commented Nov 4, 2015

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
http://ci.jujugui.org:8080//job/jujusvg/45/
Test PASSed.

serviceData := b.Services[name]
x, xerr := strconv.ParseFloat(serviceData.Annotations["gui-x"], 64)
y, yerr := strconv.ParseFloat(serviceData.Annotations["gui-y"], 64)
if xerr != nil || yerr != nil {
return nil, errgo.Newf("service %q does not have a valid position", name)
if serviceData.Annotations["gui-x"] == "" && serviceData.Annotations["gui-y"] == "" {
Copy link

Choose a reason for hiding this comment

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

While unlikely should you also support the OR case here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about it, but an x without a y still falls under the invalid clause below.

@hatched
Copy link

hatched commented Nov 4, 2015

Looks good! 👍 With some trivials

@jujugui
Copy link
Contributor

jujugui commented Nov 4, 2015

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
http://ci.jujugui.org:8080//job/jujusvg/46/
Test PASSed.

@makyo
Copy link
Contributor Author

makyo commented Nov 5, 2015

QA

  • make build
  • go run examples/generatesvg.go examples/charmworld-no-placement.yaml > ~/bundle.svg
  • Ensure no errors; view bundle.svg in the browser and see all three services placed automatically
  • Repeat with examples/charmworld-missing-placement.yaml and ensure that the one service with missing annotations is placed.

"sort"
)

// getPointOutside returns a point that is outside the hull of existing placed vertices so that an object can be placed on the canvas without overlapping others.
Copy link
Contributor

Choose a reason for hiding this comment

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

can you please limit the line length of comments to around about 70-80 characters?

@mhilton
Copy link
Contributor

mhilton commented Nov 9, 2015

👍 LGTM with minor thoughts. QA OK

@makyo
Copy link
Contributor Author

makyo commented Nov 9, 2015

Thanks all :shipit:

@jujugui
Copy link
Contributor

jujugui commented Nov 9, 2015

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
http://ci.jujugui.org:8080//job/jujusvg/47/
Test PASSed.

@jujugui
Copy link
Contributor

jujugui commented Nov 10, 2015

Build failed: Attempt to land pull request failed
build url: http://ci.jujugui.org:8080/job/jujusvg-merge/11

@jujugui
Copy link
Contributor

jujugui commented Nov 10, 2015

Build failed: Attempt to land pull request failed
build url: http://ci.jujugui.org:8080/job/jujusvg-merge/12

@jujugui
Copy link
Contributor

jujugui commented Nov 10, 2015

Build failed: Attempt to land pull request failed
build url: http://ci.jujugui.org:8080/job/jujusvg-merge/13

@jujugui
Copy link
Contributor

jujugui commented Nov 10, 2015

Build failed: Attempt to land pull request failed
build url: http://ci.jujugui.org:8080/job/jujusvg-merge/15

@jujugui
Copy link
Contributor

jujugui commented Nov 10, 2015

Status: merge request accepted. Url: http://ci.jujugui.org:8080/job/jujusvg-merge

jujugui added a commit that referenced this pull request Nov 10, 2015
Position services without annotations.

This is a backwards compatible change to place services which do not have position annotations.  It works on the case of a bundle with one/some missing service annotations as well as a bundle missing all annotations, both of which pass bundle proof.
@jujugui jujugui merged commit f084ca6 into juju:v1 Nov 10, 2015
@bac
Copy link

bac commented Nov 10, 2015

@makyo The jujusvg-merge job was never set up to use different target branches. Yesterday I didn't realize you were trying to merge into v1. After some surgery it seems to work now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants