jonnyzzz / TeamCity.GitHub

Integration of TeamCity and GitHub
216 stars 63 forks source link

Fix "guthub" typos #101

Closed msabramo closed 7 years ago

msabramo commented 8 years ago

in a way that doesn't break existing stuff that relies on the incorrectly spelled parameters.

We continue to use the old parameter names, but if we don't get a value, then we try again with the correctly spelled name.

jonnyzzz commented 8 years ago

Looks good. Could you please also include a constants file change too. You will also need to provide an updated JSP file which is able to show both older and newer values (maybe it is worth wrapping it with controller first)

msabramo commented 8 years ago

Well to be honest, I am not particularly versed in Java (haven't worked with it in year) and don't have really have an environment set up to compile and test this.

So I'm wondering if you or someone else can take this as an idea and do whatever further work is needed to get it across the finish line?

msabramo commented 8 years ago

I guess I could reverse it so that the constants file has the proper spellings and the hack I added does the revert transformation. Not sure if that's what you meant by "constants file change"?

The JSP thing is probably beyond the amount of time I can spend on this.

msabramo commented 8 years ago

Updated to make the correct spellings the primary ones and the incorrect "guthub" ones the fallback in 25ba1fa.

bszonye commented 8 years ago

Excellent, I like the approach of fixing the API spelling and fixing up any requests using the typo spelling. Perhaps we can build the plugin and install on our local TeamCity installation to test backward and forward compatibility.

mikedld commented 8 years ago

Someone has to rename "teamcilty" directory to "teamcity" as well... :)

msabramo commented 8 years ago

If @jonnyzzz merges this I'll be happy to submit another PR to fix the directory name :smile:

msabramo commented 8 years ago

I might be able to look at fixing this in a few days when I'm back at a computer.

It would be cool if this repo was configured to run CI on all PRs. That would help catch compile errors and such.

msabramo commented 8 years ago

Since this plugin implements posting status to the GitHub status API, it would be cool if the tests were run on a TeamCity server with this plugin installed and let the plugin itself post results to GitHub.

msabramo commented 8 years ago

When I last worked on this, I couldn't quite figure out how to build the plugin. See https://github.com/jonnyzzz/TeamCity.GitHub/issues/102

If I knew the appropriate ant or mvn or whatever command to build the plugin and run tests, then that would be helpful for catching errors.

msabramo commented 8 years ago

Alternatively, maybe it's not worth spending time developing this plugin if https://confluence.jetbrains.com/display/TW/Commit+Status+Publisher(bundled now with TeamCity 10 I just noticed) is now the preferred way to integrate TeamCity with GitHub status?

See https://github.com/jonnyzzz/TeamCity.GitHub/issues/106

mikedld commented 8 years ago

@msabramo, I've succeeded in building it with IntelliJ IDEA (Community edition would do). You could select Build -> Make Project from the menu, and to create a zip archive use Build -> Build Artifacts... menu item (selecting "plugin-zip").

mikedld commented 8 years ago

As for the built-in Commit Status Publisher plugin, I think it lacks flexibility this plugin provides (but I only tried it with TeamCity 9.1 where it wasn't yet built-in but was provided separately).

msabramo commented 8 years ago

Thanks, @mikedld! I can give that a shot when I'm back at a computer. Thanks!

msabramo commented 8 years ago

I just downloaded IntelliJ IDEA Community Edition 2016.2.1 and tried to build the plugin and got 100 errors, mostly a lot of things that seem to be missing.

screen shot 2016-08-07 at 2 01 25 pm

Any assistance would be appreciated.

jonnyzzz commented 8 years ago

Check 'path variables' in IDEA. Looks like it is missing TeamCity installation path there in TEAMCITY_DISTRIBUTION or similar

On Aug 7, 2016 11:02 PM, "Marc Abramowitz" notifications@github.com wrote:

I just downloaded IntelliJ Community Edition and tried to build the plugin and got 100 errors.

[image: screen shot 2016-08-07 at 2 01 25 pm] https://cloud.githubusercontent.com/assets/305268/17465263/896f152e-5ca7-11e6-9b35-b3374f002a8d.png

Any assistance would be appreciated.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/jonnyzzz/TeamCity.GitHub/pull/101#issuecomment-238108286, or mute the thread https://github.com/notifications/unsubscribe-auth/AAPpr-msQAh-EbWB147-GyD_WUyphNauks5qdkfogaJpZM4HYeo3 .

msabramo commented 8 years ago

Thanks! Ah okay I didn't realize that I needed to point TEAMCITY_DISTRIBUTION at a TeamCity install. Got things working and fixed the compile errors in 599b604.

screen shot 2016-08-07 at 3 32 26 pm

screen shot 2016-08-07 at 3 33 41 pm

jonnyzzz commented 8 years ago

Thank you for the fix. It looks like you forgot to update the actual build feature code. It seems it will not be able to continue working, once it will be expecting renamed keys but TeamCity will pass there older keys. Next, issue it the UI. In .jsp it will try using new keys, so one will not be able to edit older settings (all fields will be empty, and user' settings will be lost on 'save' click)

msabramo commented 8 years ago

Anyone want to adopt my branch and do the work that @jonnyzzz mentioned to get it fully working?

I don't see myself having time to work on this anytime soon.