Add logging of hockeyapp resultant urls, in addition to the build page links. #43

AnneTheAgile · 2017-06-06T18:14:41Z

Usually I like the console log to be the most complete source of truth. Adding logging of the hockey result urls to the console log takes me a step closer to that.

…e links.

Brantone · 2017-08-07T18:12:33Z

src/main/java/hockeyapp/HockeyappRecorder.java


                int appIndex = applications.indexOf(application);

+                logger.println("HOCKEYAPP_INSTALL_URL" + installUrl);


The catch with putting this outside the if block on line 316, is that even though the value is available it might not be set in the environment data. Would this cause confusion?

thank you for reviewing!!! I think you meant line 361 not 316 ? ie this : https://github.com/AnneTheAgile/hockeyapp-plugin/blob/f55ab778243a04de69d51fefcbfdab8d802db392/src/main/java/hockeyapp/HockeyappRecorder.java#L361
I don't see that envData is ever set at all. Am I missing something?

My preference is regardless of what future code wants, ie what usages occur of environment variables, that the log indicates what it does since it does create links in the html for the build. Those links are not available by scraping the console log, but that is what I would like to do.
https://github.com/ohoeltke/hockeyapp-plugin/search?q=envdata&type=Code&utf8=%E2%9C%93

//update ; hmm, that search is not accurate. We can't search in forks, and the jenkinsci repo is a fork. so unless merge backs are done to ohoeltke/hockeyapp-plugin , then there is no search... and the last of ohoeltke/hockeyapp-plugin was 2012, so there were no merge backs. Ugh.
//update2; Search/Find in sublime shows it does not exist in this plugin. So it apparently only exists in the calling stuffs, which are what is responsible for creating left nav url links.
I'm not sure what code that is?
But regardless, this particular plugin would not need to mind about that, since it never minded the contents of envData previously.

AnneTheAgile · 2017-09-11T21:02:37Z

Commented on @Brantone 's review.

AnneTheAgile · 2017-12-01T23:02:32Z

@Brantone what do you think of my response?

…-doc-urls-in-log-170606 # Conflicts: # src/main/java/hockeyapp/HockeyappRecorder.java

mezpahlan · 2018-08-05T11:48:01Z

Hi @AnneTheAgile, thanks so much for this enhancement. Apologies it has taken so long to get around to reviewing it. The master branch in the upstream repository has moved on quite a bit so I've updated this PR to take that into account. In doing this I have had to re-implement your changes due to a merge conflict. Could you take a look and let me know if they are still what you are expecting?

I'll mark them up with some small comments. Many thanks.

mezpahlan · 2018-08-05T11:50:54Z

src/main/java/hockeyapp/HockeyappRecorder.java


                    if (appIndex == 0) {
                        envData.add("HOCKEYAPP_CONFIG_URL", configUrl);
+                        logger.println("HOCKEYAPP_CONFIG_URL" + configUrl);


Could you double check that this is what you want to see in the log console output? It will look something like:

HOCKEYAPP_CONFIG_URL:http://your.url.to.config/

Would it be better to do something like:

logger.println("HOCKEYAPP_CONFIG_URL: " + configUrl);

So that it will output as (note the spaces and colon):

HOCKEYAPP_CONFIG_URL: http://your.url.to.config/

mezpahlan · 2018-08-05T11:54:39Z

src/main/java/hockeyapp/HockeyappRecorder.java

                    }

                    envData.add("HOCKEYAPP_CONFIG_URL_" + appIndex, configUrl);
+                    logger.println("HOCKEYAPP_CONFIG_URL_" + appIndex + ": " + configUrl);


We can have multiple config (and install) urls returned from HockeyApp. As with the environment data object we expose all urls that we are able to. Do you still want to do that with your change in the console logger? If so you can keep this and the equivalent one for installation links below. Otherwise if you just want the first configuration url feel free to delete these lines....... but I suspect you probably want them too ;).

mezpahlan

Added two comments for minor changes. Let me know if you are happy with the suggestions and either you or I can implement them.

AnneTheAgile · 2018-09-24T19:42:12Z

@mezpahlan ty so much for fixing this! Yes please I am very happy, I look forward to the next release.
Thank you for taking on maintainer role :)

douglasknudsen · 2018-09-25T15:39:41Z

Um something seems off here. Looking at the history in Git I see this commit a2cc5a4#diff-138ba9924fd1fadb6721753b08eded3b which fixed this issue. But then this commit later removed the fix 359a492#diff-138ba9924fd1fadb6721753b08eded3b Am I seeing this correctly? I tried the new release and it doesn't work, my local build of the plugin with the fix does still work though. Cheers! Douglas 'DK' Knudsen http://www.cubicleman.com

…

On Mon, Sep 24, 2018 at 3:42 PM AnneTheAgile ***@***.***> wrote: @mezpahlan <https://github.com/mezpahlan> ty so much for fixing this! Yes please I am very happy, I look forward to the next release. Thank you for taking on maintainer role :) — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#43 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAK1nNAEBw-qyLewXWPYcXEpMS-HBaBKks5ueTWXgaJpZM4Nxt--> .

mezpahlan · 2018-09-25T16:13:17Z

Quite possibly #42 undid something from #46 . Too difficult to check that on my mobile but will pick this up at home.

This PR is still in review and I don't think related to your issue so if you don't mind I'll pick this backup on #46 which was your original issue I believe and the one that you've spotted a regression in.

douglasknudsen · 2018-09-25T16:42:48Z

great. Let me know where I can help Cheers! Douglas 'DK' Knudsen http://www.cubicleman.com

…

On Tue, Sep 25, 2018 at 12:23 PM Mez Pahlan ***@***.***> wrote: Quite possibly PR 46 undid something from PR 42. Too difficult to check that on my mobile but will pick this up at home. This PR is still in review and I don't think related to your issue so if you don't mind I'll pick this backup on PR 42 which was your original issue I believe and the one that you've spotted a regression in. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#43 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAK1nFo1_oEL5ZECwoL_5GFv5XYzQCr-ks5uelh6gaJpZM4Nxt--> .

douglasknudsen · 2018-09-26T01:56:43Z

sorry for carrying this on in this thread...got the emails crossed :(

AnneTheAgile · 2018-10-01T18:58:41Z

@douglasknudsen @mezpahlan actually part of this proposed pull request did get undone in the referenced commit, right here;
359a492#diff-138ba9924fd1fadb6721753b08eded3bL772

Should I re-commit a tweak?
Will this be able to make it to the next release which maybe is next week?

mezpahlan · 2018-10-01T19:12:23Z

Hi @AnneTheAgile could you take a look at #53 . This is where I quickly reverted the change that knocked @douglasknudsen 's changes out. So as far as I am concerned the original mistake is now rectified in the master branch.

We released this as 1.3.2 feel free to update this PR to ensure that it doesn't re-revert the changes but I don't think that is necessary. I'll have a look at this PR now and then, yes, queue it up for a release.

Thanks for your support (and patience!).

…ls-in-log-170606

mezpahlan · 2018-10-01T20:19:40Z

Ok I've applied the suggestions above and am just waiting on the checks.

AnneTheAgile · 2018-10-02T15:29:00Z

ty so much @mezpahlan !

Add logging of hockeyapp resultant urls, in addition to the build pag…

f55ab77

…e links.

Brantone reviewed Aug 7, 2017

View reviewed changes

AnneTheAgile mentioned this pull request Dec 1, 2017

make HockeyApp-plugin compatible with Pipeline #32

Merged

mezpahlan added the enhancement label Jun 5, 2018

mezpahlan self-requested a review August 5, 2018 10:54

Merge branch 'master' of jenkinsci/hockeyapp-plugin into annetheagile…

baad595

…-doc-urls-in-log-170606 # Conflicts: # src/main/java/hockeyapp/HockeyappRecorder.java

mezpahlan reviewed Aug 5, 2018

View reviewed changes

mezpahlan added 2 commits October 1, 2018 20:20

Merge remote-tracking branch 'origin/master' into annetheagile-doc-ur…

edac321

…ls-in-log-170606

Add extra space to format logged string

6f06aef

mezpahlan merged commit 3a0d807 into jenkinsci:master Oct 1, 2018


		int appIndex = applications.indexOf(application);

		logger.println("HOCKEYAPP_INSTALL_URL" + installUrl);

Add logging of hockeyapp resultant urls, in addition to the build page links. #43

Add logging of hockeyapp resultant urls, in addition to the build page links. #43

Uh oh!

Conversation

AnneTheAgile commented Jun 6, 2017

Uh oh!

Brantone Aug 7, 2017

Choose a reason for hiding this comment

Uh oh!

AnneTheAgile Sep 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AnneTheAgile commented Sep 11, 2017

Uh oh!

AnneTheAgile commented Dec 1, 2017

Uh oh!

mezpahlan commented Aug 5, 2018

Uh oh!

mezpahlan Aug 5, 2018

Choose a reason for hiding this comment

Uh oh!

mezpahlan Aug 5, 2018

Choose a reason for hiding this comment

Uh oh!

mezpahlan left a comment

Choose a reason for hiding this comment

Uh oh!

AnneTheAgile commented Sep 24, 2018

Uh oh!

douglasknudsen commented Sep 25, 2018 via email

Uh oh!

mezpahlan commented Sep 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

douglasknudsen commented Sep 25, 2018 via email

Uh oh!

douglasknudsen commented Sep 26, 2018

Uh oh!

AnneTheAgile commented Oct 1, 2018

Uh oh!

mezpahlan commented Oct 1, 2018

Uh oh!

mezpahlan commented Oct 1, 2018

Uh oh!

AnneTheAgile commented Oct 2, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

AnneTheAgile Sep 11, 2017 •

edited

Loading

mezpahlan commented Sep 25, 2018 •

edited

Loading