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 pwd to docker container #108

Merged
merged 6 commits into from
Jun 3, 2020
Merged

Conversation

klarkc
Copy link
Contributor

@klarkc klarkc commented Apr 1, 2020

Allows the use of current dir .gtt.yml inside the docker container.

@coveralls
Copy link

coveralls commented Apr 1, 2020

Coverage Status

Coverage remained the same at 65.979% when pulling 9726acb on klarkc:add-docker-pwd into e059f1f on kriskbx:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 65.979% when pulling 56c9805 on klarkc:add-docker-pwd into e059f1f on kriskbx:master.

@cgdobre cgdobre self-assigned this Jun 2, 2020
Copy link
Collaborator

@cgdobre cgdobre left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution. What do you think about this idea instead to get the same result?
docker run --rm -it -v ~:/root -v $(pwd)/.gtt.yml:/.gtt.yml kriskbx/gitlab-time-tracker'
This way it wouldn't be necessary to change anything else than the documentation.

@klarkc
Copy link
Contributor Author

klarkc commented Jun 2, 2020

What do you think about this idea instead to get the same result?

Almost same result, if .gtt.yml file does not exists on host it will not throw this error:

$ docker run --rm -it -v ~:/root -v $(pwd)/.gtt.yml:/.gtt.yml kriskbx/gitlab-time-tracker report
Error parsing configuration: "/.gtt.yml"

Because, when a given path does not exists, docker creates a folder (in the host) for the volume:

$ ls -la .gtt.yml/
total 8
drwxr-xr-x  2 root   root   4096 jun  2 12:51 .
drwxr-xr-x 18 klarkc klarkc 4096 jun  2 12:51 ..

As .gtt.yml file is optional, I think this way is more painless useful for use with an alias or inside a script that run in any context.

kriskbx/gitlab-time-tracker \
--help
```

`--rm` removes the container after running, `-it` makes it interactive, `-v ~:/root` mounts your home directory to the
home directory inside the container. If you want to store the config in another place, mount another directory:
home directory inside the container, `-v $(pwd):/pwd` mounts current directory inside the container to gtt be able to read local config. If you want to store the config in another place, mount another directory:

Choose a reason for hiding this comment

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

The correct env variables is PWD, env vars are case sensitive so this will not work.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We are not using an env vars here, but the output of the pwd command. Please note use of parentheses $(pwd), not accolades ${pwd}

@juanluisbaptiste
Copy link

Hi,

This PR does not fix the issue, first, env variable names are case sensitive and the correct env variable name for the current path value is ${PWD}, ${pwd} does return an empty value. Second, the volume mount for the /root directory using "~" is still present so the docker run command will still fail. Third, you don't need to mount the complete user home directory, that is an unnecesary security risk, it is enough with mounting gtt configuration directory.

The correct way to run it would be like this, for example:


$ sudo docker run \
       --rm -it \
       -v ${PWD}/.local/share/.gtt/:/root/.local/share/.gtt \
       kriskbx/gitlab-time-tracker \
       report "ourorganization/aproject" --user=xxxx --from="2020-04-01" --to="2020-06-30"

I just tested this and it works, the current documentation does not.

@juanluisbaptiste
Copy link

Or better yet, if you specify the path to the config directory as specified above then you can use "~" to specify the path to your home directory (or ${HOME}):

$ sudo docker run \
       --rm -it \
       -v ~/.local/share/.gtt/:/root/.local/share/.gtt \
       kriskbx/gitlab-time-tracker \
       report "ourorganization/aproject" --user=xxxx --from="2020-04-01" --to="2020-06-30"

@cgdobre
Copy link
Collaborator

cgdobre commented Jul 1, 2020

Hi,

This PR does not fix the issue, first, env variable names are case sensitive and the correct env variable name for the current path value is ${PWD}, ${pwd} does return an empty value. Second, the volume mount for the /root directory using "~" is still present so the docker run command will still fail. Third, you don't need to mount the complete user home directory, that is an unnecesary security risk, it is enough with mounting gtt configuration directory.

The correct way to run it would be like this, for example:


$ sudo docker run \
       --rm -it \
       -v ${PWD}/.local/share/.gtt/:/root/.local/share/.gtt \
       kriskbx/gitlab-time-tracker \
       report "ourorganization/aproject" --user=xxxx --from="2020-04-01" --to="2020-06-30"

I just tested this and it works, the current documentation does not.

Please use the comand line as mentioned in the doc. It does work. I have tested it. just make sure to use $(pwd), not ${pwd} (parentheses not accolades).

Both /root and /pwd volumes are necessary; one for global config and one for local config, which can partially or totally override the global config.

I haven't noticed any issues with using ~. Can you tel me what error you are encountering?

@cgdobre
Copy link
Collaborator

cgdobre commented Jul 1, 2020

Or better yet, if you specify the path to the config directory as specified above then you can use "~" to specify the path to your home directory (or ${HOME}):

$ sudo docker run \
       --rm -it \
       -v ~/.local/share/.gtt/:/root/.local/share/.gtt \
       kriskbx/gitlab-time-tracker \
       report "ourorganization/aproject" --user=xxxx --from="2020-04-01" --to="2020-06-30"

If a global gtt config has not been created yet, then this complete path would cause an issue. This is why I would prefer to keep the home directory mapped into the container. You can change any of the /root and /pwd mounts in the script anyway if you wis to be more secure, but this more general script covers more use cases, probably trading off some security indeed.

@juanluisbaptiste
Copy link

Please use the comand line as mentioned in the doc. It does work. I have tested it. just make sure to use $(pwd), not ${pwd} (parentheses not accolades).

Are you completely sure that pwd in lower case works for you ? for me it does not in any linux distro, tested on ubuntu, fedora, centos:

$ echo ${pwd}

$
$ echo ${PWD}
/home/juancho
$

Shell env variables are case sensitive, all shell defined variables are uppercase according to the IEE spec. An excerpt from it:

Environment variable names used by the utilities in the Shell and Utilities volume of IEEE Std 1003.1-2001 consist solely of uppercase letters, digits, and the '_' (underscore) from the characters defined in Portable Character Set and do not begin with a digit. Other characters may be permitted by an implementation; applications shall tolerate the presence of such names. Uppercase and lowercase letters shall retain their unique identities and shall not be folded together. The name space of environment variable names containing lowercase letters is reserved for applications. Applications can define any environment variables with names from this name space without modifying the behavior of the standard utilities.

And further down you can see the shell env variables which include PWD. I really don't understand why lowercase pwd works for you, in linux.

Both /root and /pwd volumes are necessary; one for global config and one for local config, which can partially or totally override the global config.

Could you elaborate on the use of a global config file when used inside a container ?

I haven't noticed any issues with using ~. Can you tel me what error you are encountering?

One char paths like ~ (or .) are not allowed by docker as a volume path name since a long time ago. There's also an open issue on this repo about this.

@juanluisbaptiste
Copy link

If a global gtt config has not been created yet, then this complete path would cause an issue. This is why I would prefer to keep the home directory mapped into the container. You can change any of the /root and /pwd mounts in the script anyway if you wis to be more secure, but this more general script covers more use cases, probably trading off some security indeed.

Sorry it seems I do not understand the purpose of a global config file that resides on a user home directory along with the local override file. Normally, a global configuration file is installed system wide at a system path like /etc so all users of the system can make use of it and then override it with a local config file on their home directory. So if both the local and global files are on the user's home directory, the global file is global relative to what ? why using only one config file is not enough ?

@klarkc
Copy link
Contributor Author

klarkc commented Jul 8, 2020

@juanluisbaptiste You probabily not understood that the $(pwd) (which is not {pwd}) executes pwd in a sub-shell, pwd command (not variable) is part of GNU binutils, so it's already part of any linux variant.

About global/local. You might have a global config, overwritten by a local config. (Ex: you have a cross-project config inside your home directory to always query in Merge Requests, but for a given project (local directory, pwd output), you want to query only issues, so you can override that setting using the context aware file.

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.

None yet

4 participants