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 --plugin-download-directory option to examples #617

Merged
merged 4 commits into from
Nov 29, 2023
Merged

Add --plugin-download-directory option to examples #617

merged 4 commits into from
Nov 29, 2023

Conversation

aSky17
Copy link
Contributor

@aSky17 aSky17 commented Nov 27, 2023

fixes #607
Added --download-directory-option in README.md along with added the path where to install a plugin

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did

Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

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

I think ok, not 100% sure on the docker one it might confuse people as I think it defaults to the right place there, @MarkEWaite ?

Copy link
Contributor

@MarkEWaite MarkEWaite 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 pull request @aSky17 . I think that we should use a consistent path to the plugins directory in the two examples. I've proposed changes to make the new syntax work that way.

I think that the container example needs a different approach. I've suggested that we leave it unchanged in this pull request and that a later pull request be used to update the container based example.

Later pull requests might make the following additional changes:

  • Use /your/path/to/ as the directory prefix in both examples (one pull request)
  • Adapt the container example to actually update the plugins in a running container (another pull request, and likely much more complicated)

The container example pull request needs more steps than are currently listed. I was able to make the container example work with the following steps:

docker cp /your/path/to/plugins.txt <container_name>:/tmp/plugins.txt
docker exec -it <container_name> /bin/bash
jenkins-plugin-cli --plugin-file /tmp/plugins.txt
cp -r -p /usr/share/jenkins/ref/plugins/. /var/jenkins_home/plugins/.
exit

After performing those steps, the newly downloaded plugins were visible in the container image that I was running. I think those steps are complicated enough that they deserve a separate pull request.

README.md Outdated
@@ -23,21 +23,21 @@ The plugin manager downloads plugins and their dependencies into a folder so tha
Download the latest jenkins-plugin-manager jar [from here](https://github.com/jenkinsci/plugin-installation-manager-tool/releases/latest) and run it as shown below.

```bash
java -jar jenkins-plugin-manager-*.jar --war /your/path/to/jenkins.war --plugin-file /your/path/to/plugins.txt --plugins delivery-pipeline-plugin:1.3.2 deployit-plugin
java -jar jenkins-plugin-manager-*.jar --war /your/path/to/jenkins.war --plugin-download-directory /path/where/to/download/plugin --plugin-file /your/path/to/plugins.txt --plugins delivery-pipeline-plugin:1.3.2 deployit-plugin
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that we should use a consistent prefix string and use the name plugins rather than plugin since the directory plugins is where Jenkins looks for its plugins. I would prefer to include the trailing '/' character on the directory path in hopes of showing the user that the argument is a directory name and not a file name.

Suggested change
java -jar jenkins-plugin-manager-*.jar --war /your/path/to/jenkins.war --plugin-download-directory /path/where/to/download/plugin --plugin-file /your/path/to/plugins.txt --plugins delivery-pipeline-plugin:1.3.2 deployit-plugin
java -jar jenkins-plugin-manager-*.jar --war /your/path/to/jenkins.war --plugin-download-directory /your/path/to/plugins/ --plugin-file /your/path/to/plugins.txt --plugins delivery-pipeline-plugin:1.3.2 deployit-plugin

README.md Outdated
```

Alternatively, build and run the plugin manager yourself from source:

```bash
mvn clean install
java -jar plugin-management-cli/target/jenkins-plugin-manager-*.jar --war /file/path/jenkins.war --plugin-file /file/path/plugins.txt --plugins delivery-pipeline-plugin:1.3.2 deployit-plugin
java -jar plugin-management-cli/target/jenkins-plugin-manager-*.jar --war /file/path/jenkins.war --plugin-download-directory /path/where/to/download/plugin --plugin-file /file/path/plugins.txt --plugins delivery-pipeline-plugin:1.3.2 deployit-plugin
Copy link
Contributor

@MarkEWaite MarkEWaite Nov 28, 2023

Choose a reason for hiding this comment

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

I think that we should use the same form of plugins directory path in both examples. I think that once this pull request is merged we should submit a new pull request that makes the directory paths in this example consistent with the directory paths in the previous example. No need to disrupt this pull request with that change, but it would be worth doing in a later pull request.

Suggested change
java -jar plugin-management-cli/target/jenkins-plugin-manager-*.jar --war /file/path/jenkins.war --plugin-download-directory /path/where/to/download/plugin --plugin-file /file/path/plugins.txt --plugins delivery-pipeline-plugin:1.3.2 deployit-plugin
java -jar plugin-management-cli/target/jenkins-plugin-manager-*.jar --war /file/path/jenkins.war --plugin-download-directory /your/path/to/plugins/ --plugin-file /file/path/plugins.txt --plugins delivery-pipeline-plugin:1.3.2 deployit-plugin

README.md Outdated
```

If you use a [Jenkins docker image](https://hub.docker.com/r/jenkins/jenkins) the plugin manager can be invoked inside the running container via the bundled `jenkins-plugin-cli` shell script:

```bash
docker exec -it <container_name> /bin/bash
jenkins-plugin-cli --plugin-file /your/path/to/plugins.txt --plugins delivery-pipeline-plugin:1.3.2 deployit-plugin
jenkins-plugin-cli --plugin-download-directory /path/where/to/download/plugin --plugin-file /your/path/to/plugins.txt --plugins delivery-pipeline-plugin:1.3.2 deployit-plugin
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that @timja is correct that we should not adjust the download directory in this example because it is inside the Jenkins container image and that container image has an existing directory structure that we should use. The existing example does not use the existing directory and file conventions that are used in the container images. I don't thin we should change it in this pull request. Let's save that for a future pull request

Suggested change
jenkins-plugin-cli --plugin-download-directory /path/where/to/download/plugin --plugin-file /your/path/to/plugins.txt --plugins delivery-pipeline-plugin:1.3.2 deployit-plugin
jenkins-plugin-cli --plugin-file /your/path/to/plugins.txt --plugins delivery-pipeline-plugin:1.3.2 deployit-plugin

@MarkEWaite MarkEWaite changed the title Added --download-directory-option in README.md Add --plugin-download-directory option to examples Nov 29, 2023
@MarkEWaite MarkEWaite added the documentation Improvements or additions to documentation label Nov 29, 2023
Copy link
Contributor

@MarkEWaite MarkEWaite left a comment

Choose a reason for hiding this comment

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

Thanks!

@MarkEWaite MarkEWaite merged commit f426b83 into jenkinsci:master Nov 29, 2023
15 checks passed
@aSky17 aSky17 deleted the supply-directory-option branch November 29, 2023 04:42
@aSky17 aSky17 restored the supply-directory-option branch November 29, 2023 04:42
@aSky17 aSky17 deleted the supply-directory-option branch November 30, 2023 05:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sample command in README.md misses download directory option
3 participants