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.

Sign up for GitHub

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

Jump to bottom

Command-level configuration is never taken into consideration #352

Closed
lppedd opened this issue Sep 6, 2022 · 8 comments · Fixed by #353
Closed

Command-level configuration is never taken into consideration #352

lppedd opened this issue Sep 6, 2022 · 8 comments · Fixed by #353
Assignees
@gsmet

Comments

@lppedd
Copy link

lppedd commented Sep 6, 2022 •
edited

As you can see the command class name is the Quarkus-created one, not the original one.
This means commandConfigs.getOrDefault will always return the default one, from the @Cli.
Same for CommandPermissionConfig or CommandTeamConfig.

image

image

This is pretty severe imho.
@gsmet
Copy link
Member

gsmet commented Sep 6, 2022

@lppedd do you have a reproducer? Because I have tests for that. Now maybe the test infrastructure hides something so having a reproducer would certainly help.

@lppedd
Copy link
Author

lppedd commented Sep 6, 2022

@gsmet Sure, I will strip my app and upload a .zip file. Let's see what happens.

@gsmet
Copy link
Member

gsmet commented Sep 6, 2022

OK, I have a patch ready so I'll be able to check it actually fixes your issue (and hopefully add a test if it's doable).

@gsmet gsmet self-assigned this Sep 6, 2022
@lppedd
Copy link
Author

lppedd commented Sep 6, 2022

@gsmet https://drive.google.com/file/d/1CZVYncIvrhYaGKtuIog7i5tHNoJXl0UX/view?usp=sharing
Should be sufficient. Let me know when you have downloaded it.

@gsmet
Copy link
Member

gsmet commented Sep 6, 2022

@lppedd

I downloaded the reproducer.
Question: were you experimenting in dev mode by any chance?

Because I suspect the problem is only there in dev mode and is related to some changes in the Quarkus dev mode. That's why the tests are OK. I have a fix, trying to prepare a test for it.

@lppedd
Copy link
Author

lppedd commented Sep 6, 2022 •
edited

@gsmet yup! I was in test mode, you're right.
Take your time, I have simply moved the configuration in the @Cli for now.

gsmet added a commit to gsmet/quarkus-github-app that referenced this issue Sep 6, 2022
@gsmet
Normalize ArC subclasses when getting the overriding options
c741d0d 
Fixes quarkiverse#352
@lppedd
Copy link
Author

lppedd commented Sep 6, 2022

@gsmet just noticed I wrote "test mode" instead of "dev mode".
I was focusing on testing and it slipped in, sorry.

@gsmet gsmet mentioned this issue Sep 6, 2022
Merged
@gsmet gsmet closed this as completed in #353 Sep 6, 2022
@gsmet
Copy link
Member

gsmet commented Sep 6, 2022

1.11.1 has been released with a fix and is available on Maven Central.

Thanks for reporting the issue!

gsmet added a commit to gsmet/quarkus-github-app that referenced this issue Sep 6, 2022
@gsmet
Simpler fix for quarkiverse#352
8743dd8 
Rather than using the class of the command instance, we can use the
metadata directly.
Figured it out while working on the reaction strategy patch.
gsmet added a commit that referenced this issue Sep 6, 2022
@gsmet
Merge pull request #355 from gsmet/metadata
444b569 
Simpler fix for #352
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@gsmet gsmet

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Normalize ArC subclasses when getting the overriding options gsmet/quarkus-github-app
2 participants
@gsmet @lppedd