Skip to content

[WIP] Use Docker parser when manipulating Dockerfiles #1079

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.

Sign up for GitHub

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

Jump to bottom
Closed
0xmichalis wants to merge 2 commits into from
Closed

[WIP] Use Docker parser when manipulating Dockerfiles #1079

0xmichalis wants to merge 2 commits into from

Conversation

@0xmichalis
Copy link
Contributor

@0xmichalis 0xmichalis commented Feb 20, 2015

Merged this on #1103 alongside its dependencies

@0xmichalis 0xmichalis mentioned this pull request Feb 20, 2015
Closed
mfojtik
mfojtik reviewed Feb 20, 2015
View reviewed changes
pkg/build/builder/docker.go Outdated
Copy link
Contributor

@mfojtik mfojtik Feb 20, 2015

Choose a reason for hiding this comment

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

nuke this

@0xmichalis
Copy link
Contributor Author

0xmichalis commented Feb 23, 2015

@bparees @mfojtik mind reviewing?

@0xmichalis
Copy link
Contributor Author

0xmichalis commented Feb 23, 2015

ps. I am going to add unit tests for (at least) the new funcs this change introduces in a separate pr

@mfojtik
Copy link
Contributor

mfojtik commented Feb 23, 2015

@Kargakis seems like you are missing some packages that the builder depends on.

@bparees
Copy link
Contributor

bparees commented Feb 23, 2015

@Kargakis we normally do the unit tests in the same PR.

@0xmichalis
Copy link
Contributor Author

0xmichalis commented Feb 23, 2015

seems like you are missing some packages that the builder depends on.

Yes, the packages I bundle in #1103

we normally do the unit tests in the same PR.

No problem, I am going to add them here then

@0xmichalis 0xmichalis changed the title [WIP] Use Docker parser when manipulating Dockerfiles Use Docker parser when manipulating Dockerfiles Feb 23, 2015
bparees
bparees reviewed Feb 23, 2015
View reviewed changes
pkg/build/builder/docker.go Outdated
Copy link
Contributor

@bparees bparees Feb 23, 2015

Choose a reason for hiding this comment

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

I think we should err out..if we failed to do the replace, then we're not building what we're supposed to be building.

@0xmichalis 0xmichalis changed the title Use Docker parser when manipulating Dockerfiles [WIP] Use Docker parser when manipulating Dockerfiles Feb 23, 2015
@0xmichalis
Copy link
Contributor Author

0xmichalis commented Feb 24, 2015

@bparees fixed that particular Dockerfile case you sent me. If you have any other corner cases, post them here or send them via IRC so I can work on them as well.

@Kargakis
Use Docker parser when manipulating Dockerfiles
46148dc 
Add one more layer of parsing for safety by using the Docker
parser to verify the valid occurrence of a Dockerfile command.
soltysh
soltysh reviewed Feb 24, 2015
View reviewed changes
pkg/build/builder/docker_test.go
Copy link
Contributor

@soltysh soltysh Feb 24, 2015

Choose a reason for hiding this comment

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

I'd add a test to verify the output is also parsable, meaning valid Dockerfile.

Copy link
Contributor Author

@0xmichalis 0xmichalis Feb 24, 2015

Choose a reason for hiding this comment

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

It may be useful but it's out of scope of the current changes ie. command replacement vs Dockerfile validation

Copy link
Contributor

@soltysh soltysh Feb 24, 2015

Choose a reason for hiding this comment

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

Au contraire the resulting file MUST be valid Dockerfile as it's being the source of the docker build. So I insist on having such test.

Copy link
Contributor Author

@0xmichalis 0xmichalis Feb 24, 2015

Choose a reason for hiding this comment

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

I can add a test where we'll parse both the original and the new Dockerfile and compare their ASTs. Validation of the Dockerfile though is something different than what this change is supposed to do. @bparees what do you think?

Copy link
Contributor

@soltysh soltysh Feb 24, 2015

Choose a reason for hiding this comment

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

That should suffice IMHO, just running Parse on the new file should be a good start without any erros is OK.

@mfojtik
Copy link
Contributor

mfojtik commented Feb 24, 2015

@Kargakis still cannot build: cannot find package "github.com/docker/docker/builder/command

@0xmichalis
Copy link
Contributor Author

0xmichalis commented Feb 24, 2015

Merged this on #1103 alongside its dependencies.

@0xmichalis 0xmichalis closed this Feb 24, 2015
jpeeler pushed a commit to jpeeler/origin that referenced this pull request Aug 17, 2017
Squashed 'cmd/service-catalog/go/src/github.com/kubernetes-incubator/…
1cd2988 
…service-catalog/' changes from 8f07b7b..7e650e7

7e650e7 origin build: add origin tooling
f32eec2 unit test for ./pkg/rest/core/fake rest client, addresses openshift#860 Test ready to be reviewed (openshift#1113)
e388aee explicitly always prefer latest OSBAPI Version (openshift#1138)
962429e Merge branch 'pr/1135'
b6ee7ef fix rbac
cb1beb9 Merge branch 'pr/1131'
ecc5c01 Update Code of Conduct (openshift#1137)
e7c5ab3 address one more PR comment
ddcbbad address PR comments
652a83b fix test expectation to match the new error message for missing service class
33417cc address PR comments
565fccf Use the chart name instead of the namespace (openshift#1102)
bc61919 Add new terminal failure binding condition (openshift#1057)
4e642d5 Added more detailed instructions on how to setup the repo (openshift#1114)
bdaea23 update unit tests (openshift#1123)
88a9642 validate the apiserver options (openshift#1116)
b0af5fc fix whitespace in the copyright section
dee796a generated type changes
ef585c4 Rename the directory from default to defaultservicename to conform to go style guide. Wire admission controller into the apiserver
0b5d6c6 add firewall troubleshooting section (openshift#1040)
fd9e6bc Fix Typo in Events Code of Conduct (openshift#1126)
ebe6506 Fix Typo in Terminology (openshift#1128)
0038b1e Merge branch 'pr/1122'
8411f31 make deprovisioning an instance asynchronously not fall-through to synchronous deprovision (openshift#1067)
76c1d93 handle failures from list and test the not ready condition, cleanup
9241296 finish unit tests, passing
ed75774 Minor fixes based on go report card
9911e8d Add GoReport Widget (openshift#1121)
dd24e5c clean up old cruft
08276c6 generated file changes
6489d90 Implement the default plan in admission controller
a6bb576 Code: Instance/Binding parameters from secret (openshift#1079)
10bb148 Update generated files (openshift#1115)
5291e6f v0.0.15 (openshift#1118)
28a1ea6 Merge branch 'pr/1104'
bb4a2d2 Merge branch 'pr/1097'
1c14a90 push all arch images on release tags (openshift#1108)
b587b2c Improve log output for deprovision
8887561 Remove PodPreset embedding from Binding (openshift#1030)
1abdcc8 Adjust helm/tiller installation instructions (openshift#1091)
f636f99 only skip tls verify if not behind the aggregator (openshift#1101)
43b40ab controller_broker unit test bullet-proofing openshift#1077 (openshift#1099)
bb596b8 Use data store instead of database (openshift#1100)
04fa477 Implementation: Support for Bearer token auth between Service Catalog and brokers (openshift#1053)
9e46d3c refactor Jenkins e2e tests (openshift#1082)
1f0a41e remove old/misleading comments about only doing soft delete if it's "our turn--" i.e. only if the finalizer we care about is at the head of the finalizers list.
5c1d9b8 Update OSB client (openshift#1085)
a6e80ea Only do work for instances from a single queue (openshift#1074)
2bd85d6 Merge branch 'pr/1076'
e324287 Tweaks to the walkthrough for local-up-cluster
d8b7899 Add a note to the walkthrough about getting bindings when using the aggregator (openshift#1078)
ea44cf1 msg on Environment Variables to set for e2e (openshift#1070)
d15554a Merge branch 'pr/1017'
faf966e Add comment re: async race condition in integration tests
ed2e096 v0.0.14 (openshift#1071)
fc84ffd more PR feedback
283bed4 Add integration tests and some error checking; PR feedback
903a7a7 Add terminal condition for instance and do not retry failed provisions
REVERT: 8f07b7b origin: add required patches

git-subtree-dir: cmd/service-catalog/go/src/github.com/kubernetes-incubator/service-catalog
git-subtree-split: 7e650e7e39c3fc79a8ecc061cce2a70e899406ff
jpeeler pushed a commit to jpeeler/origin that referenced this pull request Feb 1, 2018
@nilebox @pmorie
Code: Instance/Binding parameters from secret (openshift#1079)
a6bb576 
* Reworked instance parameters with flexible structure

* Switch Binding parameters to a new implementation

* removed name attribute from parametersFrom

* Update the implementation according to the latest proposal
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@0xmichalis @mfojtik @bparees @soltysh @Kargakis