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

Add Push Rule service #143

Merged
merged 1 commit into from
Aug 11, 2018
Merged

Add Push Rule service #143

merged 1 commit into from
Aug 11, 2018

Conversation

jetersen
Copy link
Contributor

@jetersen jetersen commented Aug 9, 2018

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
Copy link
Contributor Author

jetersen commented Aug 9, 2018

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
Copy link
Owner

jdalrymple commented Aug 9, 2018

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

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

async createOrEdit(projectId, options) {
Copy link
Contributor Author

@jetersen jetersen Aug 9, 2018

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
Owner

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

@jdalrymple
Copy link
Owner

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

@jetersen
Copy link
Contributor Author

jetersen commented Aug 9, 2018

Sounds reasonable 😅

@jdalrymple
Copy link
Owner

lol which one makes MORE sense :P ?

@jetersen
Copy link
Contributor Author

jetersen commented Aug 9, 2018

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
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
Copy link
Contributor Author

jetersen commented Aug 9, 2018

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

@jetersen
Copy link
Contributor Author

jetersen commented Aug 9, 2018

Should be all good then 😅


class PushRule extends BaseService {

async create(projectId, options) {
Copy link
Owner

@jdalrymple jdalrymple Aug 9, 2018

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

}
}

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

@jdalrymple jdalrymple Aug 9, 2018

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
Copy link
Contributor Author

jetersen commented Aug 9, 2018

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

@jetersen
Copy link
Contributor Author

jetersen commented Aug 9, 2018

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

@@ -34,6 +34,14 @@ class PushRule extends BaseService {
return RequestHelper.get(this, `projects/${pId}/push_rule`);
}

async update(projectId, options) {
Copy link
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
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
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
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
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
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
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
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