-
Notifications
You must be signed in to change notification settings - Fork 17
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
updated readme + followup-problem in pr-body #4
Conversation
with commit gunnarmorling@4792255 the required java version was 1upped
Hey, thanks for the readme update, really appreciating it! Regarding the JFR data source, I haven't looked into this for a while. There is an issue related to this already: #3. The original idea was to include this as a Git sub module, but I suppose the structure of the JFR data source upstream repo may have changed. Probably some more investigation is needed to make this work again. |
and thank you for your quick reaction, too!
my bad, i mistyped and mentioned it as #2 above. By playing around i understood that the other repo is the source for the subfolder here. And by failing to checkout the other repo here into the subfolder i, too, was brought to learn about "git submodules". 🤓 At some point i realized that there must exist a "filebase" on your system, that made it possible for you, to use the other configs (those i mentioned i changed) in this repo "as is" ... which made me stop further attempts to tackle it after running into the next failure.
I tried to look through the rh-jmc-team/jfr-datasource commits (there was a restructuring in June 2020) but before or after i did not find the abovementioned Now when realizing that the git-submodule-solution obviously would inherent brittleness, i personally, would also be ok with a "laymans solution" here ... like duplicate the required contents here and not reference? 😇 That way the example would be working, despite changes in the jfr-datasource. If you find the time to better align the two repos, you might implement the necessary changes lateron. P.S.: If i may, i would ask you to please comment on me inserting this followup textwall here ... is it ok for you to plough through all this lines of text, or was this a burden on you. If too much ... how could i have handled it better? Email? Don't think so. I am curious what you think. |
hi gunnar, found the workaround to make it run ... i can re-use the Dockerfile.jvm you supplied in the example-service ootb
prof.farnsworth.voice good news ... i suppose? manual steps to prepare for this example:
the README.md in this PR now reflects these preparational steps. I would much more like a complete example already setup here ... so if you would like, i could setup a new PR which centralizes all these changes here. I suppose there is no legalese that would hinder such a duplication here? Would be helpful? |
Glad you got it working! Now let's see how we can make this work better OOTB.
Ah, now I begin to see where the issue lies. This shouldn't really be needed. Instead, you should get the sub module automatically when cloning this repo. But it seems I messed up when setting this up initially (as you figured sub-modules have their subtleties...). I've reworked this now, and things should work if you clone the repo via
Also shouldn't be needed as per above. That sub-module is sourced from a fork (https://github.com/gunnarmorling/jfr-datasource), which already contains the Dockerfile
Dito. So perhaps you can retry as described above and if it works update this PR to reflect that info in the README?
I think it's perfectly fine. That's what issues/PRs are for. Sure, the more concise the better, but those things are not always easy to express :) |
interesting :) my git-fu is weak, so trying this will be a learning experience 🤓
i think i will find some time this weekend to try out your suggestions! |
hmm ... so ... i had some "[email protected]: Permission denied (publickey). issues" after fiddling around i got it working after changing line 3 of .gitmodules i had to
Probably Could you change the mentioned line in .gitmodules to https-auth? I would then try If that makes it an easy one-liner, that will probably be the documentable solution? Maybe also with an additional comment explaining the error one gets, when just doing a plain ````git clone``` |
hi @gunnarmorling in my fork i changed the .gitmodules to https and was able to checkout with your commands. I reflected that changes in the readme ... also i made the prerequisites more prominently visible, which might help the uninitiated for me it is now a very pleasant "OOTB experience" to checkout the example and get it running 👍 //edit: actually i was not totally happy with the "have java 15 installed" ... maybe "make sure to use java 15 for the build" is more fitting? 🤔 also prerequisites in the "Build"-Section make more sense ... i think that is enough overthinking for today ;) |
nope, not done yet ... at least i have to check that it is not just on my machine
When i remove as a sidethought: while you are at it ... would it be feasible that you raise your fork of the jfr-datasource to java 15, too? (here and here) ... maybe not of importance, just more in line with the general java 15 "prerequisite" ... maybe all of this could be java 16 now, either? just thinking 😇 🤓 |
hi gunnar, reading your talk announcement earlier today on twitter(for P99CONF) made me remember something 💦 ... this thing here :) ... would you be interested in brushing up this example or are you already working on something more current? (i read you mentioned JfrUnit ... maybe that could be integrated here too) i am asking because while trying to re-play this setup here today i found out some more things:
I am currently re-trying all from blank checkout again to see if it now works ootb, would be nice to have another pair of eyes trying that too. Also nice would be to see it integrated / reviewed ... i am just playing along and learning but maybe it can also help others? (also i see the danger of this getting too big, so that one might refrain from poking at working on this PR) |
Thanks for the reminder, @eitsch! I've taken your README changes, slightly redacted and merged them. Thanks a lot!
Yes, this example could need some updating. I don't think JfrUnit necessarily has to be part of it, there's already a separate example project for it. But I've just logged two other issues (for upgrading to Java 16, and exploring the new JFR support in GraalVM native binaries). Any other things you'd like to see here?
Good call-out; I've just taken care of this.
Yeah, updating would be nice; we'd still have to keep a fork though, so to have the JVM-based Dockerfile, as the upstream repo only has one for GraalVM native mode.
So this should be taken care of by now, right?
Let me know if you still got issues.
This PR is merged; if you got other things you'd like to work on, knock yourself out. Thanks a lot! |
Hi Gunnar,
first: i want to state that i'd bear no hard feelings if you would refrain from tainting the commiters list with my gigantic PR ;)
second: after having read https://www.morling.dev/blog/quarkus-and-testcontainers/ i wanted to try it out and started playing with this repo.
Which led me to #2 ... after that i checked out the mentioned repo and copied the jfr-data-source content into the subdir.
Which then leads to the following error when running
docker-compose up --build
:I then tried to change line 78 in docker-compose.yaml from
dockerfile: docker/Dockerfile.jvm
todockerfile: src/main/docker/Dockerfile.native
(like shown in line 35 of same docker-compose)because that is what i could reference from the copied repos file https://github.com/rh-jmc-team/jfr-datasource/blob/main/src/main/docker/Dockerfile.native
That resulted in the following Log
This is where i am now and which brought me to the decision to put all this together in this PR 🤓
I am a bit lost where i now would need to copy something from/to and what i could do about it 🤕
Is this even the intended way of consuming the jfr-data-source⁉️