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

Fixes #279 : adding CI support for sub projects. #318

Merged
merged 1 commit into from Aug 11, 2017

Conversation

4 participants
@sch00lb0y
Copy link
Contributor

sch00lb0y commented Jul 26, 2017

Description

I have added a feature to register sub projects to CI

Related Issue

Motivation and Context

Right now, user can register only one repository to CI, so webhook are not registered to other repository. so, I have added support to register webhook to all sub repositories.

How Has This Been Tested?

https://demo08.herokuapp.com

Screenshots (if appropriate):

Types of changes

  • Bug fix
  • New feature

Checklist:

  • I have read the Contribution & Best practices Guide and my PR follows them.
  • There is a corresponding issue for this pull request.
  • Mentioned the Issue number in the pull request commit message Fixes #<number> commit message
  • There is only one commit per issue.
@fossasia-bot

This comment has been minimized.

Copy link

fossasia-bot bot commented Jul 27, 2017

Hi @sch00lb0y!

Looks like your PR has some conflicts. 😟
Could you resolve them and rebase on top of the latest upstream code ?

@sch00lb0y sch00lb0y force-pushed the sch00lb0y:cisub branch from e5774e1 to e98ada3 Jul 28, 2017

@sch00lb0y sch00lb0y changed the title [WIP] Fixes #279 : adding CI support for sub projects. Fixes #279 : adding CI support for sub projects. Jul 28, 2017

@sch00lb0y

This comment has been minimized.

Copy link
Contributor

sch00lb0y commented Jul 28, 2017

@pri22296 @imujjwal96 please review

exports.hookValidator = function (repositoryName, token) {
return new Promise(function(resolve, reject) {
token = crypter.decrypt(token);

This comment has been minimized.

@imujjwal96

imujjwal96 Jul 28, 2017

Contributor

No need to create a separate variable as it is used only once

token = crypter.decrypt(token);
request({
url: `https://api.github.com/repos/${repositoryName}/hooks?access_token=${token}`,

This comment has been minimized.

@imujjwal96

imujjwal96 Jul 28, 2017

Contributor

instead of adding a query parameter for the access token, add it in the authorization header.

url: `https://api.github.com/repos/${repositoryName}/hooks?access_token=${token}`,
headers: {
'User-Agent': 'request'

This comment has been minimized.

@imujjwal96

imujjwal96 Jul 28, 2017

Contributor

i'd say we should use Yaydoc as the user agent

}
var hooks = JSON.parse(body);
var hookurl = 'https://' + process.env.HOSTNAME + '/ci/webhook';

This comment has been minimized.

@imujjwal96

imujjwal96 Jul 28, 2017

Contributor

do not strict it to https if a deployment does not have SSL encryption the webhooks will fail. Had this problem in my deployment

hooks.forEach(function (hook) {
if (hook.config.url === hookurl) {
isRegistered = true;

This comment has been minimized.

@imujjwal96

imujjwal96 Jul 28, 2017

Contributor

break it out of loop after the hookurl is equated.

}
request({
url: `https://api.github.com/repos/${data.repositoryName}/hooks?access_token=${token}`,

This comment has been minimized.

@imujjwal96

imujjwal96 Jul 28, 2017

Contributor

same as before....

};
/**
* hookValidator is used to check wheather hook is registered to

This comment has been minimized.

@imujjwal96

imujjwal96 Jul 28, 2017

Contributor

add a single whitespace before *. All * should be in a single line

* returns clone url
*/

This comment has been minimized.

@imujjwal96

imujjwal96 Jul 28, 2017

Contributor

remove line

}

/**

This comment has been minimized.

@imujjwal96

imujjwal96 Jul 28, 2017

Contributor

add a single line description to maintain consitency


exports.getCloneUrl = function (repoName) {
var username = repoName.split('/')[0];

This comment has been minimized.

@imujjwal96

imujjwal96 Jul 28, 2017

Contributor

what's the need to split them?
username/name is same as reponame

This comment has been minimized.

@imujjwal96

imujjwal96 Jul 28, 2017

Contributor

That raises the question in the existence of this function

var subProjectId = 0; // sub projects dynamic id
var repositorySelectIds = []; // array for storing repositories select element

function registerDisableChecker () {

This comment has been minimized.

@imujjwal96

imujjwal96 Jul 28, 2017

Contributor
var registerDisableChecker = function() {
  ....
  ....
}

Maintain consistency

var repositorySelectIds = []; // array for storing repositories select element

function registerDisableChecker () {
var flag = true; // flag to check wheather any of the select is

This comment has been minimized.

@imujjwal96

imujjwal96 Jul 28, 2017

Contributor

*whether Spell check

res.redirect("/dashboard?status=registration_already");
}
} else {
var registerdHook = Promise.all([{repositoryName:repositoryName,sub:false}]

This comment has been minimized.

@imujjwal96

imujjwal96 Jul 28, 2017

Contributor

*registeredHook spell check

This comment has been minimized.

@sch00lb0y

sch00lb0y Jul 30, 2017

Contributor

updated.

@sch00lb0y sch00lb0y force-pushed the sch00lb0y:cisub branch 4 times, most recently from d5e3745 to 819acaa Jul 29, 2017

@sch00lb0y sch00lb0y removed the WIP label Jul 31, 2017

@sch00lb0y sch00lb0y force-pushed the sch00lb0y:cisub branch 5 times, most recently from cb6e15f to 0e69da6 Jul 31, 2017

@pri22296

This comment has been minimized.

Copy link
Member

pri22296 commented Aug 9, 2017

Yeah, that could work. But how will changes to the yaml file be handled? And any advantage for that approach compared to the current implementation? Coz I think it, that it would take significant amount of work, would it not?

@sch00lb0y sch00lb0y referenced this pull request Aug 9, 2017

Merged

Fixes #360 Use auth middleware and move github requests #363

6 of 7 tasks complete
@imujjwal96

This comment has been minimized.

Copy link
Contributor

imujjwal96 commented Aug 9, 2017

What decides the source for subproject in the current implementation?

@imujjwal96

This comment has been minimized.

Copy link
Contributor

imujjwal96 commented Aug 9, 2017

What we can do is.

  • Add .yaydoc.yml in a master project
  • Register the project to Yaydoc CI
  • On registering, the yaml file is checked for subprojects. If subprojects are defined, we register these projects and their hooks.
  • Thereafter on each commit we check if a change is made in yaydoc.yml (maybe store a key in repository database (subprojects: true)). If the change is made, we check if subprojects are added or removed. On this change, we add or remove hooks for those repositories
@sch00lb0y

This comment has been minimized.

Copy link
Contributor

sch00lb0y commented Aug 9, 2017

how will we handle if the user doesn't have access to sub project? if we implement this we have to change for everything like reading theme, doc path for normal project generation as well. will it be possible for us to change the whole implementation within the small time span and make the documentation for all FOSSASIA project up and running ?

@imujjwal96

This comment has been minimized.

Copy link
Contributor

imujjwal96 commented Aug 9, 2017

  • What decides the source for subprojects in the current implementation?
  • What decides the branch for subprojects in the current implementation?
@imujjwal96

This comment has been minimized.

Copy link
Contributor

imujjwal96 commented Aug 9, 2017

Moving forward with the current implementation, when i register BrimeNotes/documentation and imujjwal96/brimeAPI, i get application error

@sch00lb0y

This comment has been minimized.

Copy link
Contributor

sch00lb0y commented Aug 9, 2017

I have checked the condition. it is working for me. I have cleared the db could you remove the webhook and try again ?

@sch00lb0y sch00lb0y force-pushed the sch00lb0y:cisub branch from 165869b to 927b23e Aug 10, 2017

@sch00lb0y

This comment has been minimized.

Copy link
Contributor

sch00lb0y commented Aug 10, 2017

@imujjwal96 I have checked by forking your repository. it is working for me. please confirm for me

@@ -96,3 +96,75 @@ exports.deleteHook = function (name, hook, accessToken, callback) {
}
});
};

/*
* hookValidator is used to check whether hook is registered to repository or not

This comment has been minimized.

@imujjwal96

imujjwal96 Aug 11, 2017

Contributor

*Check whether hook is registered to repository or not


/*
* hookValidator is used to check whether hook is registered to repository or not
* @param repositoryName: name of the repository

This comment has been minimized.

@imujjwal96

imujjwal96 Aug 11, 2017

Contributor

@param name: full_name of the repository
Follow the style used while commenting other functions in the project

}, function(error, response, body) {
if (response.statusCode !== 201) {
console.log(response.statusCode + ': ' + response.statusMessage);
resolve({status: false, body:body});

This comment has been minimized.

@imujjwal96

imujjwal96 Aug 11, 2017

Contributor

Why haven't you used reject here?

This comment has been minimized.

@sch00lb0y

sch00lb0y Aug 11, 2017

Contributor

Promise.all will go to catch if we reject. we have to remove the previously registered webhook if any of the webhook fails. I didn't write the function but for the future, I wrote

This comment has been minimized.

@imujjwal96

imujjwal96 Aug 11, 2017

Contributor

Can we not remove the registered hook from catch?

This comment has been minimized.

@sch00lb0y

sch00lb0y Aug 11, 2017

Contributor

catch won't return the array of the previously registered value. it just returns the value where catch has been executed

}

/**
* registerHook is used to register hook to the respository

This comment has been minimized.

@imujjwal96

imujjwal96 Aug 11, 2017

Contributor

Follow practices already used in the project

@@ -113,4 +126,44 @@ $(function() {
$("#disableModal").modal('hide');
});
});

var registerSearchEvents = function (id) {

This comment has been minimized.

@imujjwal96

imujjwal96 Aug 11, 2017

Contributor

add documentation

@sch00lb0y sch00lb0y force-pushed the sch00lb0y:cisub branch 4 times, most recently from 743583f to 7f0c8fe Aug 11, 2017

@sch00lb0y sch00lb0y force-pushed the sch00lb0y:cisub branch from 7f0c8fe to d1e04a5 Aug 11, 2017

@sch00lb0y

This comment has been minimized.

Copy link
Contributor

sch00lb0y commented Aug 11, 2017

@imujjwal96 updated.

@imujjwal96
Copy link
Contributor

imujjwal96 left a comment

Looks good! Great job

@fossasia-bot fossasia-bot bot removed the needs-review label Aug 11, 2017

@mariobehling mariobehling merged commit a4aff5c into fossasia:master Aug 11, 2017

2 checks passed

codacy/pr Good work! A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment