-
-
Notifications
You must be signed in to change notification settings - Fork 300
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
Add Push Rule service #143
Conversation
did a npm test with the newly added createOrEdit test and works on our instance. Guess the Travis CI is having a bad day 😢 |
Yea travis is still having issues and im not sure why. Still waiting to hear back from them: travis-ci/travis-ci#9965 |
src/services/PushRule.js
Outdated
return RequestHelper.post(this, `projects/${pId}/push_rule`, options); | ||
} | ||
|
||
async createOrEdit(projectId, options) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can remove this if you consider this too much but I thought it be useful otherwise devs would have to implement this all over.
Would it make sense to include the createOrEdit functionality within the edit function with an option? |
OR have it included in the create function with an overwrite option? |
Sounds reasonable 😅 |
lol which one makes MORE sense :P ? |
Well create seems more reasonable since it throws error if it exists thats more understandable then edit...
edit throws:
|
Sounds good to me! So just add an option to overwrite Wooo. This is awesome. Hopefully i can get it to release ASAP. Just waiting on the travis team for an update. |
Just to be clear, your suggestion was removing createOrEdit and use overwrite option? |
Should be all good then 😅 |
src/services/PushRule.js
Outdated
|
||
class PushRule extends BaseService { | ||
|
||
async create(projectId, options) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
async create(projectId, options = { overwrite: false }) {
src/services/PushRule.js
Outdated
} | ||
} | ||
|
||
return RequestHelper.post(this, `projects/${pId}/push_rule`, options); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be cleaner to do:
...
const { overwrite, ...args } = options;
if (overwrite) {
const pushRule = await this.show(projectId);
if (pushRule) return this.edit(projectId, args)
}
return RequestHelper.post(this, `projects/${pId}/push_rule`, args);
Overwrite seems like a bad word since a put only updates. It does not overwrite |
overwrite would be delete and post, while, update would be post or put |
src/services/PushRule.js
Outdated
@@ -34,6 +34,14 @@ class PushRule extends BaseService { | |||
return RequestHelper.get(this, `projects/${pId}/push_rule`); | |||
} | |||
|
|||
async update(projectId, options) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this avoid the options parameter and seems cleaner at the same time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, my only reservation is the name, i feel like its close in meaning to edit no?
Let me step back and think. When we create a new push rule, we want to handle the case where the push rule may already exist. In that case we want to edit that already existing push rule and update its values. Generally though, we usually don't create something and find out it already exists. We usually try and edit something and find out it doesnt exist. So what i suggest, which it kinda against what we spoke about above, it adding this functionality to the edit function, with an option to upsert/create/ensureExists or something like that. I think that would be clearer than an update function (which would get confused with edit) and overwrite (which probably wouldnt be used as often) Thoughts? |
I like |
All of those functions are async, they just dont require the function prefix because the its contained in the return and doesnt include an |
This reads horribly in edit 😞 I definitely prefer upsert just as an method async edit(projectId, options) {
const pId = encodeURIComponent(projectId);
const { upsert, ...args } = options;
if (upsert) {
const pushRule = await this.show(projectId);
if (!pushRule) return this.create(projectId, args)
}
return RequestHelper.put(this, `projects/${pId}/push_rule`, args);
} |
This is a little cleaner async edit(projectId, { upsert, ...options } ={}) {
const pId = encodeURIComponent(projectId);
if (upsert) {
const pushRule = await this.show(projectId);
if (!pushRule) return this.create(projectId, options)
}
return RequestHelper.put(this, `projects/${pId}/push_rule`, args);
} |
# [3.8.0](3.7.0...3.8.0) (2018-08-14) ### Bug Fixes * **api:** Updating project members all function to include the inherited members. [#141](#141) ([e081a16](e081a16)) * **package:** update [@semantic-release](https://github.com/semantic-release)/npm to version 5.0.0 ([dc9748d](dc9748d)) * **package:** update [@semantic-release](https://github.com/semantic-release)/npm to version 5.0.1 ([12b6ca1](12b6ca1)), closes [#139](#139) * **package:** Updating packages and fixing [#140](#140) due to a babel update ([04d1769](04d1769)) ### Features * Add push rule service ([#143](#143)) ([395f83c](395f83c)) * Add transfer a project to a new namespace ([#145](#145)) ([87e9f55](87e9f55))
🎉 This PR is included in version 3.8.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
# [3.8.0](jdalrymple/gitbeaker@3.7.0...3.8.0) (2018-08-14) ### Bug Fixes * **api:** Updating project members all function to include the inherited members. [jdalrymple#141](jdalrymple#141) ([e081a16](jdalrymple@e081a16)) * **package:** update [@semantic-release](https://github.com/semantic-release)/npm to version 5.0.0 ([dc9748d](jdalrymple@dc9748d)) * **package:** update [@semantic-release](https://github.com/semantic-release)/npm to version 5.0.1 ([12b6ca1](jdalrymple@12b6ca1)), closes [jdalrymple#139](jdalrymple#139) * **package:** Updating packages and fixing [jdalrymple#140](jdalrymple#140) due to a babel update ([04d1769](jdalrymple@04d1769)) ### Features * Add push rule service ([jdalrymple#143](jdalrymple#143)) ([395f83c](jdalrymple@395f83c)) * Add transfer a project to a new namespace ([jdalrymple#145](jdalrymple#145)) ([87e9f55](jdalrymple@87e9f55))
# [3.8.0](jdalrymple/gitbeaker@3.7.0...3.8.0) (2018-08-14) ### Bug Fixes * **api:** Updating project members all function to include the inherited members. [jdalrymple#141](jdalrymple#141) ([e081a16](jdalrymple@e081a16)) * **package:** update [@semantic-release](https://github.com/semantic-release)/npm to version 5.0.0 ([dc9748d](jdalrymple@dc9748d)) * **package:** update [@semantic-release](https://github.com/semantic-release)/npm to version 5.0.1 ([12b6ca1](jdalrymple@12b6ca1)), closes [jdalrymple#139](jdalrymple#139) * **package:** Updating packages and fixing [jdalrymple#140](jdalrymple#140) due to a babel update ([04d1769](jdalrymple@04d1769)) ### Features * Add push rule service ([jdalrymple#143](jdalrymple#143)) ([395f83c](jdalrymple@395f83c)) * Add transfer a project to a new namespace ([jdalrymple#145](jdalrymple#145)) ([87e9f55](jdalrymple@87e9f55))
Love the newly added
updatePushRule
but it felt lacking when you wanted to enforce push rule for all projects and gitlab API was lacking acreateOrEdit
function and would return null with status code 404 when no push rules were created.