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

Add developer joy ! #159

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add developer joy ! #159

wants to merge 1 commit into from

Conversation

ggrebert
Copy link

@ggrebert ggrebert commented Oct 23, 2024

  • DevService
  • Auto configure jsch logs
  • @JSchSession annotation:
    • configurable by properties
    • named sessions
    • auto connect/disconnect

Next step:

  • Add @JSchChannel annotation
  • auto mount a directory in the devService

@ggrebert ggrebert requested a review from a team as a code owner October 23, 2024 10:31
/**
* Enable the JSch DevService.
*/
@WithDefault("true")
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps it should be defaulted to false to avoid spinning up a service when it's not needed?

Copy link
Author

Choose a reason for hiding this comment

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

Most of extensions in quarkus (datasources, message queues, ...) and quakiverse (mailpit, ...) active the devservices by default,
But I can change this behavior if you want.

Copy link
Member

Choose a reason for hiding this comment

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

That's fine I think. My only concern is about spinning up images when they are not needed, but that's okay I guess

Copy link
Author

Choose a reason for hiding this comment

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

there is a reuse property which is set to false by default, I can set it to true.
the reason why i have choosed to set the reuse to false by default is:

  • when reuse is false, the ryuk auto remove the container
  • when reuse is true, TestContainer does not run ryuk so, the container is never stop and removed.
    To me, this use case is usefull for containers which need lots of resources and when we work with multiple services

Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to have that explanation added to the docs so we don't lose it

Copy link
Member

Choose a reason for hiding this comment

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

About the reuse flag, I think it should be set to false by default. Spinning up a new SSH server is fast and the devservice should IMHO live as long as the app is running

Copy link
Member

Choose a reason for hiding this comment

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

TBH... I think I would tend to set it to false by default. The reason is that this extension is extremely low level and could be used by a lot of other extensions to connect to SSH.

And in that case, you won't define a client via config and you won't need a dev service.

I'm also a bit torn on the config thing. I would have expected people to use it to connect to all sorts of stuff. Not sure being able to connect to only one server is useful.

Now, maybe I'm missing something as to how/when this library is used. I just want to make sure we don't make the extension impractical for using it as a low level component.

Copy link
Member

Choose a reason for hiding this comment

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

Even better: it could be true but the devservice is only started if an injection point is found AND no host is configured for the session

Copy link
Author

Choose a reason for hiding this comment

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

Context examples:

  • Historical use of Axway to transfer files from server to many servers
    The named session annotation for simple use cases
    And JSchSessions.fromName(String sessionName) method for dynamic use cases

    Due to the interop interface contracts between french telecom operators, the usage of Axway is mandatory for lots of applications

  • S3 to SFTP, SFTP to SFTP: create simple and dynamic config at runtime with environment variables

Copy link
Author

Choose a reason for hiding this comment

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

we can also create a new extension that depends on this one. (ex: quarkus-jsch-simple)
then we can keep the low level

@gastaldi
Copy link
Member

gastaldi commented Oct 23, 2024

That is really cool! Great job! I'll try it locally before merging and report my findings. Perhaps @ppalaga would be interested too?

@ggrebert
Copy link
Author

Thank you <3

@ggrebert ggrebert force-pushed the dev-joy branch 2 times, most recently from 1b8c66b to 71fe783 Compare October 23, 2024 12:19
* Mock the JSch session in dev and test mode.
*/
@WithDefault("true")
public boolean mock();
Copy link
Member

Choose a reason for hiding this comment

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

I find this confusing. It's not really mocking the session, but returning the default session instead.

But I am probably missing the whole picture here. Can you elaborate on the use case this is needed?

Copy link
Author

Choose a reason for hiding this comment

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

new doc:

/**
 * Mock the JSch session in dev and test mode:
 * This is useful for testing purposes with devservices where
 * we re-use the default session configuration generated by the devservice.
 */

Is it OK for you ?

Copy link
Member

Choose a reason for hiding this comment

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

I understand the reason, but I guess it may be frustrating if you have set up 2 sessions and when you quarkus dev your app, you see that the second session is always connecting to the default session. Perhaps start a different SSH Server for each configured session?

Copy link
Member

Choose a reason for hiding this comment

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

Also, start an SSH server only if the host is not defined (this is the same behavior as when defining the URL for Datasources)

Copy link
Author

Choose a reason for hiding this comment

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

I think it is fixed

@ggrebert ggrebert force-pushed the dev-joy branch 2 times, most recently from 02ab602 to 3326cca Compare October 25, 2024 13:00
@ggrebert ggrebert force-pushed the dev-joy branch 3 times, most recently from 7ed704c to 013e5ea Compare October 25, 2024 17:11
public class JSchResource {

@JSchSession("doudou")
Copy link
Member

Choose a reason for hiding this comment

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

I think using io.smallrye.common.annotation.Identifier would be better. Plus, you shouldn't need to use @JSchSession given that the injection type expected is already a Session

Copy link
Author

Choose a reason for hiding this comment

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

Yes I can do it with @Identifier.

But I want to be consistent with the future works: create a session by a simple annotation is cool, but it will be even better to create a channel.

I have already thought about it:

package io.quarkus.jsch;

import java.lang.annotation.ElementType;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
import java.lang.annotation.Target;

@Retention(RetentionPolicy.RUNTIME)
@Target({ ElementType.FIELD })
public @interface JschChannel {

    /**
     * The channel type.
     */
    String value();

    /**
     * The session name.
     */
    String session() default JSchSession.DEFAULT_SESSION_NAME;

}

And i havent find a way to do it with just a simple naming annotation.
I wanted to be consistent with all this extension's annotations. But yes, we can use @Identifier for sessions and for the channels we can see it later and use a custom one.

I'll let you choose the solution that seems the cleanest to you.

Copy link
Author

Choose a reason for hiding this comment

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

The injections types for this future annotation are:

  • com.jcraft.jsch.Channel
  • com.jcraft.jsch.ChannelSftp
  • com.jcraft.jsch.ChannelShell
  • com.jcraft.jsch.ChannelExec

* DevService
* Auto configure jsch logs
* JSchSession annotation:
   * configurable by properties
   * named sessions
   * auto connect/disconnect
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