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.

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

add .s2i/environment to getting-started for OOB OpenShift S2I support #81

Merged
merged 2 commits into from
Mar 28, 2019
Merged

add .s2i/environment to getting-started for OOB OpenShift S2I support #81

merged 2 commits into from
Mar 28, 2019

Conversation

vorburger
Copy link
Contributor

No description provided.

@vorburger vorburger mentioned this pull request Mar 8, 2019
Closed
vorburger added a commit to vorburger/quarkusio.github.io that referenced this pull request Mar 8, 2019
@vorburger
add new openshift-s2i-guide
6ff4ca1 
see quarkusio/quarkus-quickstarts#81
@vorburger vorburger mentioned this pull request Mar 8, 2019
Merged
vorburger added a commit to vorburger/quarkus that referenced this pull request Mar 8, 2019
@vorburger
add getting-started-openshift-s2i (fixes quarkusio#1306)
e3a8fb4 
quarkusio#1306

see also quarkusio#1305

requires quarkusio/quarkus-quickstarts#81

and then to actually publish the link to this new guide in the menu
it needs quarkusio/quarkusio.github.io#126
This was referenced Mar 8, 2019
Merged
Closed
@vorburger
Copy link
Contributor Author

NB: This is with <uberJar>true, because that's what works best as-is today.

An alternative without that is proposed for discussion in #82.

@vorburger vorburger mentioned this pull request Mar 8, 2019
Closed
cescoffier
cescoffier requested changes Mar 9, 2019
View reviewed changes
getting-started-openshift-s2i/pom.xml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@vorburger
Copy link
Contributor Author

#87 will cause a (minor) conflict with this PR; ping me to rebase and fix that when #87 is merged.

@vorburger
Copy link
Contributor Author

@cescoffier what do we want to do re. this one?

This was referenced Mar 19, 2019
Closed
Closed
@vorburger vorburger changed the title add getting-started-openshift-s2i [WIP] add getting-started-openshift-s2i Mar 22, 2019
@vorburger
Copy link
Contributor Author

@cescoffier and I discussed yesterday that he really much prefers having #82 folded into this one, so I've cherry-picked 002fc14 from there to here. We also concluded that we do NOT want a separate getting-started-openshift-s2i but instead will just add the (required..) .s2i/environment to getting-started. I'll therefore move it according in this PR, and re-test it all together.

FYI @tqvarnst @siamaksade @geoand @kameshsampath

@vorburger vorburger mentioned this pull request Mar 22, 2019
Closed
@vorburger
add S2I support to getting-started/
9243d59 
via .s2i/environment with suitable options,
and by fixing the JAR's finalName in POM,

see quarkusio/quarkus#1305

Credit goes to @gastaldi for "inspiration" for this taken from Launcher.
@vorburger vorburger changed the title [WIP] add getting-started-openshift-s2i add getting-started-openshift-s2i Mar 22, 2019
@vorburger
Copy link
Contributor Author

@cescoffier @gastaldi @geoand this is ready for your review and merge from my side now.

@geoand
Copy link
Collaborator

geoand commented Mar 22, 2019

Looks good to me 👍

cescoffier
cescoffier requested changes Mar 22, 2019
View reviewed changes
getting-started/pom.xml Outdated Show resolved Hide resolved
@cescoffier
Copy link
Member

We would need some words in the Kubernetes - OpenShift guide (main quarkus repository)

@vorburger
Copy link
Contributor Author

vorburger commented Mar 22, 2019 •
edited
Loading

We would need some words in the Kubernetes - OpenShift guide (main quarkus repository)

yep, that's proposed over in quarkusio/quarkus#1353

@vorburger vorburger mentioned this pull request Mar 26, 2019
Closed
@vorburger vorburger changed the title add getting-started-openshift-s2i add .s2i/environment to getting-started for OOB OpenShift S2I support Mar 26, 2019
@vorburger
Copy link
Contributor Author

vorburger commented Mar 26, 2019 •
edited
Loading

@cescoffier please re-review, I expect you'll like this better now... 🛩️

@gastaldi check out how this avoids fixing <build><finalName> now.

vorburger added a commit to vorburger/quarkus that referenced this pull request Mar 26, 2019
@vorburger
rm <build><finalName> doc from openshift-s2i-guide.adoc
4742cc3 
see quarkusio/quarkus-quickstarts#81
gastaldi
gastaldi approved these changes Mar 26, 2019
View reviewed changes
Copy link
Contributor

@gastaldi gastaldi left a comment

Choose a reason for hiding this comment

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

LGTM

@cescoffier
Copy link
Member

Does it work with the latest release or does it required SNAPSHOT?

As this PR is open against master, it has to tun against the latest release. But I guess it's not the case. So we would need to re-open it against the development branch.

@vorburger
Copy link
Contributor Author

Does it work with the latest release or does it required SNAPSHOT?

it works just fine against the latest release as-is! (I tested it.)

As this PR is open against master, it has to tun against the latest release.

Given above, is it OK to keep it for master then? Merge?

So we would need to re-open it against the development branch.

FYI GitHub Trick: PR Edit (upper right hand corner) let's you change the into branch - useful.

@cescoffier cescoffier merged commit 7a81840 into quarkusio:master Mar 28, 2019
@jmtd jmtd mentioned this pull request Apr 11, 2019
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@geoand geoand geoand left review comments

@kameshsampath kameshsampath kameshsampath left review comments

@cescoffier cescoffier cescoffier requested changes

@gastaldi gastaldi gastaldi approved these changes

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
@vorburger @geoand @cescoffier @gastaldi @kameshsampath