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

Add upstreams and targets #99

Merged
merged 1 commit into from
Nov 19, 2017

Conversation

dgarlitt
Copy link
Contributor

This adds upstream and target capabilities to kongconfig as requested
in: #69

This change closes #69

@dgarlitt dgarlitt force-pushed the add-upstreams-and-targets branch from 548dbce to d404300 Compare November 15, 2017 18:20
@CyExy
Copy link
Contributor

CyExy commented Nov 15, 2017

Great stuff, thanks for the PR. I'll pull it down and review properly before merging.

@dgarlitt
Copy link
Contributor Author

dgarlitt commented Nov 15, 2017

@CyExy I noticed that the checks were failing and looked at the TravisCi build. It seems that Kong is not being started on localhost. Any idea why that may be?

@CyExy
Copy link
Contributor

CyExy commented Nov 16, 2017

I'll look into the Tarvis issue.

@CyExy
Copy link
Contributor

CyExy commented Nov 16, 2017

Please could you rebase on the master, that fixes travis build

This adds upstream and target capabilities to kongfig as requested
in: mybuilder#69
@dgarlitt dgarlitt force-pushed the add-upstreams-and-targets branch from d404300 to ed9173f Compare November 16, 2017 17:24
@dgarlitt
Copy link
Contributor Author

@CyExy Let me know if there are any changes to be made.

@CyExy
Copy link
Contributor

CyExy commented Nov 17, 2017

Sorry for the delay, and thanks for rebasing. I'm going to look through it first thing tomorrow morning.

Copy link
Contributor

@CyExy CyExy left a comment

Choose a reason for hiding this comment

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

Good idea of breaking up the code a bit and adding the parsers dir.

There was a couple of issues, as I was looking into the issues I fixed them as well. patches

The first patch is fixing the issue with updating the target weight, and when you run and update the tests snapshots you can see the difference.

And the second patch resolves the issue with ensure: removed and setting the weight to 0. Currently when you set the ensure removed then it will try to remove the target every time you run the apply. The second patch includes a fix for adding a target without attributes.

You have added the package-lock.json I would think that it is best to move over to npm or add the package-lock.json to .gitignore because having two lock files in the project would just create confusion. I personally do prefer yarn over npm but not against changing it.

return t.target === target.target;
});

return !!existing && diff(target.attributes, existing.attributes);
Copy link
Contributor

Choose a reason for hiding this comment

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

Currently changing the target weight doesn't trigger an update; return !!existing && diff(target.attributes, existing.attributes).length == 0 (notice the .length == 0) fixes the issue

@CyExy CyExy merged commit ed9173f into mybuilder:master Nov 19, 2017
@CyExy
Copy link
Contributor

CyExy commented Nov 19, 2017

@dgarlitt I would like to send you a kongfig t-shirt, if interested please email me your postal address, size etc to sten [at] mybuilder.com add your github username as well so I know its you :)

58737 2f402996 2fnnuownvr8rvnefw 2fkongfig-mockup

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

Successfully merging this pull request may close these issues.

Support new upstream api
2 participants