Skip to content

Add Push Rule service#143

Merged
jdalrymple merged 1 commit into
jdalrymple:masterfrom
jetersen:push_rule_service
Aug 11, 2018
Merged

Add Push Rule service#143
jdalrymple merged 1 commit into
jdalrymple:masterfrom
jetersen:push_rule_service

Conversation

@jetersen

@jetersen jetersen commented Aug 9, 2018

Copy link
Copy Markdown
Contributor

Love the newly added updatePushRule but it felt lacking when you wanted to enforce push rule for all projects and gitlab API was lacking a createOrEdit function and would return null with status code 404 when no push rules were created.

@jetersen

jetersen commented Aug 9, 2018

Copy link
Copy Markdown
Contributor Author

did a npm test with the newly added createOrEdit test and works on our instance. Guess the Travis CI is having a bad day 😢

@jdalrymple

jdalrymple commented Aug 9, 2018

Copy link
Copy Markdown
Owner

Yea travis is still having issues and im not sure why. Still waiting to hear back from them: travis-ci/travis-ci#9965

Comment thread src/services/PushRule.js Outdated
return RequestHelper.post(this, `projects/${pId}/push_rule`, options);
}

async createOrEdit(projectId, options) {

@jetersen jetersen Aug 9, 2018

Copy link
Copy Markdown
Contributor Author

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.

@jdalrymple

Copy link
Copy Markdown
Owner

Would it make sense to include the createOrEdit functionality within the edit function with an option?

@jdalrymple

Copy link
Copy Markdown
Owner

OR have it included in the create function with an overwrite option?

@jetersen

jetersen commented Aug 9, 2018

Copy link
Copy Markdown
Contributor Author

Sounds reasonable 😅

@jdalrymple

Copy link
Copy Markdown
Owner

lol which one makes MORE sense :P ?

@jetersen

jetersen commented Aug 9, 2018

Copy link
Copy Markdown
Contributor Author

Well create seems more reasonable since it throws error if it exists thats more understandable then edit...

{
    "error": "Project push rule exists"
}

edit throws:

{
    "message": "404 Push Rule Not Found"
}

@jdalrymple

Copy link
Copy Markdown
Owner

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.

@jdalrymple jdalrymple changed the title add push rule service Add Push Rule service Aug 9, 2018
@jetersen

jetersen commented Aug 9, 2018

Copy link
Copy Markdown
Contributor Author

Just to be clear, your suggestion was removing createOrEdit and use overwrite option?

@jetersen

jetersen commented Aug 9, 2018

Copy link
Copy Markdown
Contributor Author

Should be all good then 😅

Comment thread src/services/PushRule.js Outdated

class PushRule extends BaseService {

async create(projectId, options) {

@jdalrymple jdalrymple Aug 9, 2018

Copy link
Copy Markdown
Owner

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 }) {

Comment thread src/services/PushRule.js
}
}

return RequestHelper.post(this, `projects/${pId}/push_rule`, options);

@jdalrymple jdalrymple Aug 9, 2018

Copy link
Copy Markdown
Owner

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);

@jetersen

jetersen commented Aug 9, 2018

Copy link
Copy Markdown
Contributor Author

Overwrite seems like a bad word since a put only updates. It does not overwrite

@jetersen

jetersen commented Aug 9, 2018

Copy link
Copy Markdown
Contributor Author

overwrite would be delete and post, while, update would be post or put

Comment thread src/services/PushRule.js Outdated
return RequestHelper.get(this, `projects/${pId}/push_rule`);
}

async update(projectId, options) {

Copy link
Copy Markdown
Contributor Author

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.

Copy link
Copy Markdown
Owner

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?

@jdalrymple

Copy link
Copy Markdown
Owner

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?

@jetersen

Copy link
Copy Markdown
Contributor Author

I like upsert as a wording for create or edit 👍 Perhaps upsert as a method instead of a parameter to avoid having edit as async function?

@jdalrymple

Copy link
Copy Markdown
Owner

All of those functions are async, they just dont require the function prefix because the its contained in the return and doesnt include an await line :)

@jetersen

Copy link
Copy Markdown
Contributor Author

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);
  }

@jdalrymple

Copy link
Copy Markdown
Owner

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);
  }

@jdalrymple jdalrymple merged commit 395f83c into jdalrymple:master Aug 11, 2018
@jetersen jetersen deleted the push_rule_service branch August 14, 2018 16:49
jdalrymple pushed a commit that referenced this pull request Aug 14, 2018
# [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))
@jdalrymple

Copy link
Copy Markdown
Owner

🎉 This PR is included in version 3.8.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

mdsb100 pushed a commit to mdsb100/node-gitlab that referenced this pull request Aug 17, 2018
# [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))
mdsb100 pushed a commit to mdsb100/node-gitlab that referenced this pull request Aug 17, 2018
# [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))
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.

2 participants