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

feat: Deploy Eclipse Che on a virtual Kubernetes cluster #2719

Merged
merged 5 commits into from
May 28, 2024

Conversation

maheshrajrp
Copy link
Contributor

@maheshrajrp maheshrajrp commented Apr 14, 2024

What does this pull request change?

Addition of Support for Che with vCluster

What issues does this pull request fix or reference?

eclipse-che/che#22845

Specify the version of the product this pull request applies to

7.82

Pull Request checklist

The author and the reviewers validate the content of this pull request with the following checklist, in addition to the automated tests.

  • Any procedure:
    • Successfully tested.
  • Any page or link rename:
    • The page contains a redirection for the previous URL.
    • Propagate the URL change in:
  • Builds on Eclipse Che hosted by Red Hat.
  • the Validate language on files added or modified step reports no vale warnings.

@maheshrajrp maheshrajrp requested review from deerskindoll and a team as code owners April 14, 2024 14:40
@maheshrajrp maheshrajrp requested a review from svor April 14, 2024 14:40
Copy link

github-actions bot commented Apr 14, 2024

🎊 Navigate the preview: https://6656f23d1575547e71c0dfaa--eclipse-che-docs-pr.netlify.app 🎊

Copy link

github-actions bot commented Apr 14, 2024

Click here to review and test in web IDE: Contribute

@maheshrajrp maheshrajrp changed the title Che on vCluster feat: Deploy Eclipse Che on a virtual Kubernetes cluster Apr 14, 2024
@maheshrajrp
Copy link
Contributor Author

@tolusha @deerskindoll @ibuziuk reopened PR with the latest changes.

@tolusha
Copy link
Contributor

tolusha commented Apr 15, 2024

@deerskindoll
Could you confirm that the article has to be split into smaller ones?
If so, I can help with that.

this is a question for @maheshrajrp.

@maheshrajrp
Copy link
Contributor Author

@deerskindoll
Could you confirm that the article has to be split into smaller ones?
If so, I can help with that.

this is a question for @maheshrajrp.

hi @tolusha
im not sure if we could split this, the best we could do is move just the keycloak setup. the client id, secret creation are still needed. i fear it might do more harm than it's worth it ?

in any case, do you have thoughts or suggestions @tolusha @deerskindoll ?

@deerskindoll
Copy link
Contributor

I'm worried about the readability and clarity of the process. It's really long and involves many components that go beyond the process of installation. I'd prefer to shift keyclok setup to a separate process and add it as a prerequisite to the installation itself. Let me check with another writer for a second opinion.

@maheshrajrp I need you to include the fix for the missing attribute issue, the build and verify PR workflow won't passs otherwise. There's also a potential issue with the usage of [source,shell] but I need to look into it more.

@maheshrajrp
Copy link
Contributor Author

I'm worried about the readability and clarity of the process. It's really long and involves many components that go beyond the process of installation. I'd prefer to shift keyclok setup to a separate process and add it as a prerequisite to the installation itself. Let me check with another writer for a second opinion.

@maheshrajrp I need you to include the fix for the missing attribute issue, the build and verify PR workflow won't passs otherwise. There's also a potential issue with the usage of [source,shell] but I need to look into it more.

@deerskindoll can you give some hints on what you mean by attributes issues or point me to some documentation? it would really help.. first-time contributor here, sorry and thanks !!

@deerskindoll
Copy link
Contributor

No worries. I left the suggestion in your original PR. {domain_name} and {keycloak_host} are causing an asciidoc issue because asciidoc treats them as attributes.

You need to add a backslash \ to avoid this error: \{domain_name} and {{keycloak_host}`

@deerskindoll
Copy link
Contributor

There are two more issues with the code blocks:

Can you please fix these issues?

@maheshrajrp
Copy link
Contributor Author

maheshrajrp commented Apr 16, 2024

@deerskindoll . Thanks for the hints. I can understand. Will fix those issues. Give me a day or two (as right now I am in the middle of a career crisis), I'll pick it up and complete it asap.

@deerskindoll
Copy link
Contributor

@deerskindoll . Thanks for the hints. I can understand. Will fix those issues. Give me a day or two (as right now I am in the middle of a career crisis), I'll pick it up and complete it asap.

Take you time, there's no pressure here.

@tolusha
Copy link
Contributor

tolusha commented May 6, 2024

Hello @maheshrajrp
How are you?

@maheshrajrp
Copy link
Contributor Author

Hi @tolusha ,
I've made some changes to the PR. Can we trigger build now to validate ?

@tolusha
Copy link
Contributor

tolusha commented May 21, 2024

@deerskindoll
Could you have a look please?

@deerskindoll
Copy link
Contributor

@deerskindoll Could you have a look please?

The pipeline ran successfully and PR preview is available now. I noticed that in the navigation, the articles are organized like this:

image

That's not correct, is it? Virtual kubernetes installation, cloud installation and local installation are 3 separate processes on the same level, aren't they?

@maheshrajrp
Copy link
Contributor Author

@deerskindoll Could you have a look please?

The pipeline ran successfully and PR preview is available now. I noticed that in the navigation, the articles are organized like this:

image

That's not correct, is it? Virtual kubernetes installation, cloud installation and local installation are 3 separate processes on the same level, aren't they?

Yes, those need to separated. If you guys could give an hint about what went wrong, I'll fix it. I did some comparison and still fail to identify the root issue. It would be great if you could help on that. Thanks

@deerskindoll
Copy link
Contributor

@deerskindoll Could you have a look please?

I'm on it but I need some time to figure out how to best present the procedure. I'll try splitting it into 2-3 smaller chunks for better readability.

Co-authored-by: Anatolii Bazko <abazko@redhat.com>
@tolusha
Copy link
Contributor

tolusha commented May 27, 2024

@maheshrajrp
Sounds good for me.

@deerskindoll

Copy link
Contributor

@deerskindoll deerskindoll left a comment

Choose a reason for hiding this comment

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

last small change.

Copy link
Contributor

@deerskindoll deerskindoll left a comment

Choose a reason for hiding this comment

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

the procedure is very complex and not easy to split, let's keep it as is.

…al-kubernetes-cluster.adoc


Typo Changes

Co-authored-by: Jana Vrbkova <jvrbkova@redhat.com>
@maheshrajrp
Copy link
Contributor Author

@tolusha @deerskindoll made the requested change.

@tolusha
Copy link
Contributor

tolusha commented May 28, 2024

@maheshrajrp
Thank you a lot for the contribution.
We really appreciate that.

@tolusha tolusha merged commit be21279 into eclipse-che:main May 28, 2024
5 of 6 checks passed
@maheshrajrp
Copy link
Contributor Author

@maheshrajrp
Thank you a lot for the contribution.
We really appreciate that.

Thanks for merge @tolusha. And sorry for the delay.. got caught in middle of some picky situations..

Additionally, I'll look into contributing to a few other documentations, too.

Thanks for your review and support !! Was insightful..

@maheshrajrp maheshrajrp deleted the install_in_vcluster branch June 5, 2024 05:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants