Skip to content

Conversation

@Azhrei251
Copy link

@Azhrei251 Azhrei251 commented Aug 2, 2018

This PR allows users to specify a version number to upload their builds to. This allows for more than one build to be referenced to the same version.

The issue initially faced was uploading a new version every build was quickly getting out of hand, and finding specific historic builds (those released to the public) in the app was becoming difficult. Uploading several builds to the same version cuts down on the number of internal versions being shown in-app.

@mezpahlan mezpahlan self-requested a review August 5, 2018 10:44
@mezpahlan
Copy link

Hi @Azhrei251 thanks for this pull request. Can you do a few things for me please. Could you add in a little description of the problem this solves? I can have a guess by looking at the code but I don't want to make assumptions about it. You should be able to edit the description in PR.

Secondly the PR has merge conflicts that will need to be resolved you can do this by updating your feature branch with the latest from the master branch in the upstream repository.

Thirdly are there any tests that you could add for this? I don't really use HokeyApp like this but instead upload a single build to a pre-existing project so I want to have some more confidence in the form of a unit test.

And finally, I'm sure you've noticed but, the class HockeyappRecorder is becoming a bit of a mega class and I don't want to add anything unnecessary to it. Could you break out the enum and the inner class into separate files please. Actually regarding the HTTP verb enum, I'd be surprised if there isn't something existing in the Apache HTTP library that we depend on.

I do appreciate that the rest of the plugin code base isn't like this but I'd like to refactor it in the future so that it is easier to work with so need to start laying down some standards in the interim.

Anyways, thanks for the help!

@Azhrei251
Copy link
Author

Hey @mezpahlan, thanks for the response. For your comments:

  1. Added a description to the PR.
  2. Merge conflicts resolved. The wonders of whitespace.
  3. I've added a unit test for the version upload case, I hope that helps.
  4. Moved HttpInfo to a separate file, removed the enum and used HttpPost/HttpPut.METHOD_NAME constants instead.

Let me know if there's anything else.

Thanks!

Copy link

@mezpahlan mezpahlan left a comment

Choose a reason for hiding this comment

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

Hi @jenkinsci/code-reviewers would one of you mind taking a look at the comment I left in VersionCreation.java in this PR. The PR looks promising but I was wondering about what to do regarding maintaining backward compatibility and the strategies to deal with that. Thanks.


@DataBoundConstructor
public VersionCreation(String appId) {
public VersionCreation(String appId, String versionCode) {

Choose a reason for hiding this comment

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

Hi @jenkinsci/code-reviewers we've received this PR that looks promising, however I want to check what the right thing to do is with this public method now that it is changing. I gather we don't want to break things for anyone that relies on the API exposed publicly by this plugin. Would it be better to add an overload in this case and default the second parameter? Many thanks.

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.

Formally it breaks binary compatibility. I woul recommend having a compat constructor which may be marked as depeecated

@Azhrei251
Copy link
Author

I've added an overload constructor as recommended

@mezpahlan mezpahlan merged commit 84a6f93 into jenkinsci:master Aug 24, 2018
@MikolajKakol
Copy link

Hi, unfortunately this PR is not backward compatible. Now I'm getting:

java.lang.IllegalArgumentException: Arguments to class net.hockeyapp.jenkins.uploadMethod.VersionCreation have to be explicitly named
	at org.jenkinsci.plugins.structs.describable.DescribableModel.instantiate(DescribableModel.java:277)
	at org.jenkinsci.plugins.structs.describable.UninstantiatedDescribable.instantiate(UninstantiatedDescribable.java:168)

My current configuration was:
uploadMethod: versionCreation('xxx')
now as I understand I need to pass versionCode. Which is pain, because I don't have access to that in Jenkinsfile.

@MikolajKakol
Copy link

I have no idea how jenkins plugins works but I think that old constructor shouldn't be deprecated and it still should have this @DataBoundConstructor annotation.

@mezpahlan
Copy link

Hi @MikolajKakol I've created a bug in the issue tracker. Let's continue the discussion over there for now until we can create another PR. Thanks.

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.

4 participants