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

Various fixes to the PR panel #4556

Merged
merged 3 commits into from
Mar 27, 2017
Merged

Conversation

sunix
Copy link
Contributor

@sunix sunix commented Mar 24, 2017

  1. The user already using the oauth Github token to create the fork, upload the ssh key and create the PR so it makes sense to git push with GH token. Changing AddSshForkRemoteStep with AddHttpForkRemoteStep in GitHubContributionWorkflow.java
  2. It doesn't work if we change the local branch to use. Base upstream branch to merge into will change. This is because the panel is supposed to work from a Factory where attribute "base upstream branch to merge to" is set. We should fix it somehow: user should be able to choose the upstream branch to merge into. Also, We shouldn't update this branch when selecting another local branch. https://www.youtube.com/watch?v=H4_cJJWQ5DY

https://issues.jboss.org/browse/CHE-106

Changelog

Various fixes to the PR panel

sunix added 2 commits March 24, 2017 17:11
…id having to upload the SSH key

https://issues.jboss.org/browse/CHE-106

Signed-off-by: Sun Seng David Tan <sutan@redhat.com>
…g from a PR factory and have empty source branch project metadata

https://issues.jboss.org/browse/CHE-106

Signed-off-by: Sun Seng David Tan <sutan@redhat.com>
return;
}

vcsServiceProvider.getVcsService(context.getProject()) //
Copy link
Contributor

Choose a reason for hiding this comment

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

could you please remove // at the end of lines. It's unusual for che such kind of formatting technique.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you elaborate what's wrong with that ?
If the default formatting rules in Che doesn't let the Che users format their code the way they want then we are going nowhere: #339

Copy link
Contributor

@skabashnyuk skabashnyuk Mar 27, 2017

Choose a reason for hiding this comment

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

Can you elaborate what's wrong with that ?

Nothing. It's just looks weird to add your personal preference formatting and to be
surprised that other committees may not understand it.

About formatting.
This is a formatting style for intellij. Developed by @florent and @evoevodin
https://github.com/codenvy-legacy/dev/blob/cded88bbe9b6cf006ac5ba5b99ca9069a87109cc/che-codestyle/src/main/resources/che-codestyle-intellij-new.xml

Docker command to run without Intellij

docker run -ti \
            -u root \
            --net="host" \
            --privileged=true \
            -e DISPLAY=${DISPLAY} \
            -v /tmp/.X11-unix:/tmp/.X11-unix \
            -v ${HOME}/.Xauthority:/home/developer/.Xauthority \
            -v  {path to sources}:/cheformat/sources ksmster/che-format

If you don't like this format we can discuss it in che-dev mailist.

Copy link
Contributor Author

@sunix sunix Mar 27, 2017

Choose a reason for hiding this comment

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

OK I will remove the // and open a discussion in che-dev.
About the docker command, it's not that I don't trust you but ... I won't run that with root :)

Copy link
Contributor

Choose a reason for hiding this comment

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

heh

@voievodin
Copy link
Contributor

Could you please describe changes a bit.
Does it mean that you will always use https remote even if you have ssh key?

@sunix
Copy link
Contributor Author

sunix commented Mar 27, 2017

@evoevodin I've updated the description
It means that in all the cases we already have the GH token that we already use for create the fork, create the PR (uploading the ssh key) etc ... so it makes sense for the Github workflow to reuse it to Git push.
It means that we don't always upload the ssh key even if we already can push with https and the GH OAuth Token

… comming from a PR factory and have empty source branch project metadata

Signed-off-by: Sun Seng David Tan <sutan@redhat.com>
@sunix sunix merged commit f6ec0aa into eclipse-che:factory_migration Mar 27, 2017
@codenvy-ci
Copy link

Build # 2285 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/2285/ to view the results.

@JamesDrummond JamesDrummond added this to the 5.6.0 milestone Mar 30, 2017
@JamesDrummond JamesDrummond mentioned this pull request Apr 2, 2017
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants