-
Notifications
You must be signed in to change notification settings - Fork 58
feat(jfrog): add JFrog vscode extension, CLI completion and docker support #115
Conversation
jfrog-oauth/run.sh
Outdated
if [ "${CONFIGURE_CODE_SERVER}" == "true" ]; then | ||
while ! [ -x /tmp/code-server/bin/code-server ]; do | ||
counter=0 | ||
if [ $counter -eq 30 ]; then |
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 an arbitrary timeout, this feels a bit short (slow network, hiccup, etc). It'd nice to communicate between modules since we're already assuming the path (/tmp/code-server
). Perhaps we could have the code-server module write an install status file instead, like touch /tmp/.code-server_installing
, touch /tmp/.code-server_done
and touch /tmp/.code-server_failed
. Then we simply check for the presence of these files while we wait.
Also, are we sure that once this file is present, running it won't fail? Could it be incomplete (still copying), missing a resource file it requires, etc? (This also paints a case for the aforementioned suggestion.)
As a perhaps better alternative, it would be pretty nice to be able to define script dependencies in the modules, e.g. code server module script runs before this one, etc.
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.
yes, I agree. Having a mechanism for dependence is probably the best base. For now we can modify the code-server module to create status files. I am not sure how we can check the insatlling status.
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.
Also, should we condition the part of the module or the whole module? I think your idea can be extended to have a common input for all modules depends_on = module_name
where we check for file /tmp/.${module_name}-success
before we start runing coder_script
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.
If there's a way to reference another modules resource, I think that would work well for this use case. But I'm not sure how terraform would behave in this case. Maybe we need both a module and a script level dependency so that terraform can exit early if only one module is used without its dependency?
Or do we automatically pull in dependencies without them being explicitly declared?
Anyway, as for how the script part might look, maybe something like this?
// code-server
resource "coder_script" "install" {}
// my-module
resource "coder_script" "takes_a_while" {}
resource "coder_script" "finalize" {
depends_on = [code-server.coder_script.install, coder_script.takes_a_while]
}
Again, if we can refer like this, it'd be ideal, otherwise we may need to use strings and define the dependency in runtime? Perhaps it would be a good idea to think about what other use-cases we have, and maybe write a proposal? This might also tie a little bit into coder/coder#11131?
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.
In the meantime, I'd rather not use status files, but instead wait for the process to be started or a successful curl or something.
I do like us eventually using depends_on
though or some other way to wait.
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 now I am going to merge them without any modification until we have a formal way to resolve #93.
npm
>= 9
Will this break old versions of the module? If so, would like to hold off until we add support for versioning |
@bpmct I tried to make it so as not to break old versions by not introducing any new required variables or renaming previous variables. |
.npmrc
to work with version >=9Related coder/coder#11338