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

Restore TCK pom profiles and fix its README to refer to MP JWT 2.0 #266

Conversation

sberyozkin
Copy link
Contributor

No description provided.

@sberyozkin sberyozkin added this to the MPJWT-2.0 milestone Sep 8, 2021
@radcortez
Copy link
Contributor

Why do we need these profiles? Is someone actually using them?

@sberyozkin
Copy link
Contributor Author

Hi @radcortez I can imagine the container one being used. I suppose we can clean a few things in the TCK module, if say we remove the container-full profile then may be should also remove a full tck file as right now everyone uses the base one. I guess it could be done in a post Jakarta API alignment release, say 3.0 ?

@radcortez
Copy link
Contributor

radcortez commented Sep 8, 2021

Yes, but how are these profiles used?

Because they are defined locally in our pom, so and looking into their definitions it seems we expect a jar from a container (with the implementation) so we can run it directly from our build. Is that your understanding?

Right now, we never had that use case, and I doubt that we want to do it, because it may potentially create a chicken/egg problem when you update the API, but the implementation you are running against is not updated yet.

@sberyozkin
Copy link
Contributor Author

@radcortez Yeah, I don't really mind much if they stay or if they don't. This is not related to the Jakarta alignment story though which was my point - there are a few other things which can be removed. Would it work for you if I create a cleanup TCK module issue for 3.0 ? (as mentioned in the other issue, I'd not mind remove a few other bits from there, from Readme etc)

@Emily-Jiang
Copy link
Member

The change looks okay to me. For further clean up, I think it is better to raise an issue to get people aware of this. Maybe they can comment whether they need this profile or not.

@radcortez
Copy link
Contributor

If you insist in readding them, fine, but I think they are not being used (and have never been used).

@Emily-Jiang
Copy link
Member

If you insist in readding them, fine, but I think they are not being used (and have never been used).

@radcortez how do you know it is not being used :o? Not sure why it was added in the first place.

@radcortez
Copy link
Contributor

Well, they are local to this project. Profiles are not included in dependencies, and if we are not using them here, I don't see how someone can be using them.

Unless they fork the project and make it part of a build somehow, but it is not the correct way of doing this. This has been in the project since the start when things were still undefined. My gut feeling is that this was added to possible add implementation builds with the API (as we have in Jakarta, but MP doesn't work that way)

@sberyozkin
Copy link
Contributor Author

Hi @radcortez OK, let me commit to do a TCK module clean up in a dedicated issue, I'll open an issue shortly, if we drop them now then the whole README has to be cleaned up, and may be some files removed. Thank you

@sberyozkin
Copy link
Contributor Author

@radcortez @Emily-Jiang See #267

@sberyozkin sberyozkin merged commit 3eb4459 into microprofile:master Sep 8, 2021
@sberyozkin sberyozkin deleted the restore_tck_profiles_and_fix_readme branch September 8, 2021 17: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