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

remove references to jvm-build-service #5132

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

tnevrlka
Copy link
Member

  • jvm-build-service is currently not being used by anyone
  • It is going to undergo major breaking changes and is going to be removed for now until the changes are done

Split into two commits:

  • first is removing the references in most of the repository.
  • second is removing references to jbs in components/sandbox/tiers

The second commit contains files created by ksctl as mentioned in https://github.com/redhat-appstudio/infra-deployments/blob/main/components/sandbox/tiers/README.adoc leading to thousands of newly added lines

@openshift-ci openshift-ci bot requested review from chmeliik and mmorhun December 12, 2024 19:43
Copy link

openshift-ci bot commented Dec 12, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: tnevrlka
Once this PR has been reviewed and has the lgtm label, please assign jgwest for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@tnevrlka
Copy link
Member Author

/hold

Copy link
Member

@chmeliik chmeliik left a comment

Choose a reason for hiding this comment

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

The manual changes LGTM, just not sure why something ended up generating 5000 lines of new stuff 😕

@@ -13,16 +13,19 @@ resources:
- tiertemplate-appstudio-clusterresources-593233715-593233715.yaml
- tiertemplate-appstudio-clusterresources-809836689-809836689.yaml
- tiertemplate-appstudio-contributor-1817914940-1817914940.yaml
- tiertemplate-appstudio-contributor-3265408548-3265408548.yaml
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need such changes?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, it looks really messy, but I'm not aware of anything I can do about it.

The source of truth and the only thing I'm changing is in components/sandbox/tiers/src/appstudio.
We need such changes, because there are references to jvm-build-service and I'm removing them, see an example here

After changes are made in the directory, a command is ran as described in the README which autogenerates majority of the changes in 6c6aed6.

This seems to me to indeed be the intended workflow, see #5137 as an example

Copy link
Contributor

Choose a reason for hiding this comment

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

This is normal. Changing one access in a role will case a generation of a new role file, thus explaining all those new lines. This is how kubesaw works to deploy the new tier.

@tnevrlka
Copy link
Member Author

tnevrlka commented Dec 17, 2024

The manual changes LGTM, just not sure why something ended up generating 5000 lines of new stuff 😕

It scares me a little bit, but it kinda makes sense to me when you consider this PR which only changes the maintainer file and has 1225 newly added lines.

I've changed four files which could result in 4 times as many lines.
Sanity check: 1225*4=4900, I have around 4854. Q.E.D.

It seems like it might be a good idea to get someone from infra to take a look at this though

- jvm-build-service is currently not being used by anyone
- It is going to undergo major breaking changes and is going to be removed for now until the changes are done
- There are mentions of jvm-build-service in components/sandbox/tiers
- Manually remove the references from `components/sandbox/tiers/src`
- Run `ksctl generate nstemplatetiers --source ./src --out-dir ./production` as mentioned in README (same for staging) which automatically generates many new files
Copy link

openshift-ci bot commented Dec 20, 2024

@tnevrlka: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/appstudio-e2e-tests 430f589 link true /test appstudio-e2e-tests

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

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

Successfully merging this pull request may close these issues.

5 participants