Skip to content

Conversation

@atikhono
Copy link

HOCKEYAPP_INSTALL_URL and HOCKEYAPP_CONFIG_URL are not set for Pipelines because the appropriate interface is not implemented. Implement that.

Original PR: #22

In scope of JENKINS-33310

@atikhono atikhono changed the title Support setting HOCKEYAPP_INSTALL_URL for Pipelines [JENKINS-33310] Support setting HOCKEYAPP_INSTALL_URL for Pipelines Jan 16, 2018
@atikhono
Copy link
Author

@reviewbybees

@ghost
Copy link

ghost commented Jan 16, 2018

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

@atikhono
Copy link
Author

@Brantone up

Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

lgtm

@douglasknudsen
Copy link

This looks awesome! We have been looking forward to this for a bit and had a SO post on it https://stackoverflow.com/questions/45963651/hockeyapp-plugin-and-jenkins-pipeline-and-ha-ockey-app-url

@oleg-nenashev
Copy link
Member

ping @Brantone

@douglasknudsen
Copy link

Is there maybe another approver available? I'm willing to test and provide input if that helps. The change does look good to me, though I'm not a jenkins plugin whiz, I do recall a Jesse Glick perhaps mentioning that this was the issue in some comments somewhere perhaps.

Also, this addresses https://issues.jenkins-ci.org/browse/JENKINS-37157 as well as 33310

@douglasknudsen
Copy link

douglasknudsen commented Mar 13, 2018

Tested this the past couple days. I see the availability of HOCKEYAPP_INSTALL_URL__n_ within a pipeline step in the plugin java code. I do NOT see env.HOCKEYAPP_INSTALL_URL__n_ though in the Jenkins arena. 😢 ( using Jenkins 2.111 with all pipeline plugins up-to-date)

@douglasknudsen
Copy link

@mezpahlan Perhaps you can have a peek at this? Glad to see some life brought back to this plugin

@mezpahlan
Copy link

Hi @douglasknudsen . I was just looking at this as you pinged. Great timing! I'm pretty new to the Jenkins plugin ecosystem and being a maintainer so I do want to take a little time to go over this. My first impressions are yup good enhancement. But I want to familiarise myself a bit more with the functionality (it's not something I use in my own HockeyApp usage). I'm confident we can get this merged quickly, however.

@mezpahlan
Copy link

mezpahlan commented Jun 5, 2018

Hi @douglasknudsen sorry for the delay. Yup, looks great and fixes what looks like a real annoyance for people. You mentioned that env.HOCKEYAPP_INSTALL_URL__n_ was not present. Would you say that is gives us cause to look again at this given that the HOCKEYAPP_INSTALL_URL__n_ environment variable was present?

@mezpahlan mezpahlan added the bug label Jun 5, 2018
@douglasknudsen
Copy link

@mezpahlan I fully understand, I am in similar case without much plugin writing knowledge at all. I can test this though.

Yes, the hockeyplugin variables are not added to the environment variables at all. I can see the plugin log the needed variables to the 'console log' of a build, but they are not available in the environment variables of the pipeline job so we can not say email the hockey build URL at the end of a pipeline based job. I wonder if the base code of this plugin is using too old of a version of Jenkins plugin APIs?? just a stab in the dark. Again, I can test any changes you or @atikhono make.

@mezpahlan
Copy link

OK, I think I understand a bit better now. I will see if I can reproduce this on my end and look to a fix. Would the steps to reproduce be something like: Upload to HockeyApp, Receive response from API, print build url to console log, ensure same url is available via the environment variables?

I did bump up the version of Jenkins in the last update so perhaps it is worth trying these changes out with the latest from master.

@douglasknudsen
Copy link

Yes, sounds about right. I would expect "sh 'printenv' " in a pipeline after the hockey plugin is complete to surface the hockey properties exposed. This we I can email the direct URL to people that feel important or post it to Slack and such. :)

I tested a direct master pull and build as well as the released version you recently let out, still not showing for me.

@mezpahlan
Copy link

mezpahlan commented Jun 8, 2018 via email

@mezpahlan
Copy link

Hi @douglasknudsen sorry for the delay. I do see the appropriate environment variables being set when the HockeyApp upload completes and I add sh 'printenv' as the next item in the pipeline.

I tested this against Jenkins 2.121.1 running on Ubuntu 18.04. Here are the environment variables it prints out for me:

HOCKEYAPP_CONFIG_URL_0
HOCKEYAPP_INSTALL_URL
HOCKEYAPP_INSTALL_URL_0
HOCKEYAPP_CONFIG_URL

Given this I am quite tempted to merge this in, what do you think?

@douglasknudsen
Copy link

@mezpahlan apologies, on holiday and missed the emails....I can confirm that this change does work for me using Jenkins 2.129. Not sure why it didn't before when I last tested it! likely a cached build or some nonsense. Lets merge it!

@mezpahlan
Copy link

No worries, I've been a bit busy too with the World Cup this month. Thank you so much for all your help. Very much appreciated!

@mezpahlan mezpahlan merged commit a2cc5a4 into jenkinsci:master Jul 5, 2018
@douglasknudsen
Copy link

Thanks! and Thanks @atikhono for getting this started!

@jakob-grabner
Copy link

Any info when this will be released? Really looking forward to this 😃

@mezpahlan
Copy link

@jakob-grabner Apologies this has taken so long. That's my fault. I'll try and get some time this weekend to release it. If I don't please keep pushing me to do so. Apologies again.

@mezpahlan
Copy link

@jakob-grabner Released as 1.3.1. Apologies again for the delay.

@jakob-grabner
Copy link

Thanks, that's awesome :)

@mezpahlan
Copy link

Apologies @douglasknudsen, @jakob-grabner, @atikhono the change in a subsequent commit reverted this change so it isn't released with 1.3.1. I think this was caused by a conflict resolution that I did on #42 .

I'll spin up a branch to fix this and release this tonight.

@douglasknudsen
Copy link

douglasknudsen commented Sep 25, 2018 via email

mezpahlan added a commit to mezpahlan/hockeyapp-plugin that referenced this pull request Sep 25, 2018
mezpahlan added a commit that referenced this pull request Sep 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants