-
Notifications
You must be signed in to change notification settings - Fork 194
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
[ARQ-1321] Support for HTTPS in URLs injected with @ArquillianResource #43
Conversation
If you haven't noticed, we're having a little naming/sementics discussion on twitter around this: https://twitter.com/aslakknutsen/status/308875252237213696 :) |
…nts to this annotation.
|
@Documented | ||
@Retention(RUNTIME) | ||
@Target({ElementType.FIELD, ElementType.PARAMETER}) | ||
public @interface Secured { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency, I think @secure may be more appropriate (without the 'd'). Thoughts ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But you don't secure it here, you expect to have secured one injected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bartoszmajsak you may close this PR, it's super outdated and this has already been implemented here: https://github.com/arquillian/arquillian-showcase/tree/master/extensions/arquillian-smart-url.
However, this should probably be available in the core instead of the showcase project.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was just wondering what is the status of this, as it looks like a pretty valid functionality. I will look into the extension then. Thanks for your contribution
@aslakknutsen Have you had the time to check this PR? |
@bartoszmajsak what we can do with this? We integrate smart-url in core? |
@lordofthejars should we? |
@bartoszmajsak in my opinion I would merge this PR and avoid more complicated things. |
I wouldn't merge right away. I would compare with the extension from the showcase first. + take into consideration the discussion on twitter mentioned above. |
@aslakknutsen Have you already had the time to check this PR? |
@aslakknutsen @bartoszmajsak @lordofthejars @starksm64 this issue is the one that was recently discussed during a call about the TCK moving to Arquillian. Having the HTTPS URL is required by various tests. I understand there's a ton of difficulty around this and it's not trivial to review, but has there been any progress here in those 8 years? |
Not that I'm aware of. @aslakknutsen can you chime in? |
I won't comment on @secure vs @secured and the likes, nor have I been much involved in the project the last 5 ish years, how ever... On top of my head I could see the current impl as provided here as a possible fallback/backwards compatible version, the reason being that the Injection of URLs are intended to come from the Container to avoid hard coding them in the test case. The way this is implemented now would force the Test case to only run on Containers that e.g. use 8443 as a port. Following the ProtocolMetadata SPI that is returned by the DeployableContainer on deploy, I would have rather added either a HTTPSContext or added a secure field to the HTTPContext (and thereby adding two HTTPContexts to the ProtocolMetadata). The @secured simply switch between the two contexts, but port numbers etc are still provided by the DeployableContainer. |
@arjantijms Is there a requirement in terms of what the unit test class is doing? I looked at some threads the TCK list but did not see this issue mentioned. |
@starksm64 for the Client-Cert test in Servlet, a request to the HTTPS URL is needed. The test for the |
Replaced with #474 since this PR could not be updated via the github interface |
This change is