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

Document naming conventions & Naming alignment #368

Merged
merged 4 commits into from
Oct 30, 2024
Merged

Conversation

jfaltermeier
Copy link
Contributor

@jfaltermeier jfaltermeier commented Oct 29, 2024

Please check the Naming.md first https://github.com/eclipse-theia/theia-cloud/pull/368/files#diff-458dcaeb7b373b080fb85240d56af356cc0cbc6d054f90fb92ef87dcf4748cb5

Let me know what you think about the changes.

Contributed on behalf of STMicroelectronics

Helm Chart Changes

eclipse-theia/theia-cloud-helm#78

@jfaltermeier jfaltermeier force-pushed the jf/rename branch 2 times, most recently from d87764e to 45dd773 Compare October 29, 2024 12:58
@jfaltermeier jfaltermeier changed the title WIP Document naming conventions & Naming alignment Document naming conventions & Naming alignment Oct 29, 2024
@jfaltermeier jfaltermeier marked this pull request as ready for review October 29, 2024 13:26
documentation/Naming.md Show resolved Hide resolved
@@ -1,5 +1,5 @@
{
"name": "theiacloud-monitor",
"name": "theia-cloud-monitor",
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to take care of something here regarding the release to npm?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The npm package is already published under a name following the conventions: https://www.npmjs.com/package/@eclipse-theiacloud/monitor-theia.
This one here is the vsix, which hasn’t been published to open-vsx yet, so we should be fine.

Copy link
Contributor

@lucas-koehler lucas-koehler left a comment

Choose a reason for hiding this comment

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

Hi @jfaltermeier, thanks for aligning the naming and the documentation. This saves a lot of wondering what is the correct spelling ✨

The changes already look pretty good to me. I have two remarks inline. One tiny suggestion and one hint regarding consistency in Kubernetes label key naming. Please have a look.

Contributed on behalf of STMicroelectronics
jfaltermeier and others added 3 commits October 29, 2024 14:56
Co-authored-by: Simon Graband <[email protected]>
Co-authored-by: Lucas Koehler <[email protected]>
Contributed on behalf of STMicroelectronics
@jfaltermeier
Copy link
Contributor Author

We have to use THEIACLOUD for env variables, because Theia messes with variables starting with THEIA_

I addressed this + the comments above now

@jfaltermeier
Copy link
Contributor Author

This is the theia code: https://github.com/eclipse-theia/theia/blob/f29c3d04241b32791462c7027938abeb887f8140/packages/core/src/node/messaging/ipc-protocol.ts#L67

Copy link
Contributor

@lucas-koehler lucas-koehler left a comment

Choose a reason for hiding this comment

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

Thanks for the updates. LGTM now ✨

@jfaltermeier jfaltermeier merged commit 78b4b58 into main Oct 30, 2024
11 checks passed
@jfaltermeier jfaltermeier deleted the jf/rename branch October 30, 2024 09:31
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.

3 participants