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

[WIP] "configGroups" literal in configuration group operations URL (old PR #780) #922

Closed
wants to merge 4 commits into from

Conversation

fgalan
Copy link
Member

@fgalan fgalan commented Oct 7, 2020

Issue #752

This is a port of old PR #780 (authored by @Jagatjot). That PR was closed by the author but in order to not losing his valuable work, we are opening this one. If somebody is interested in continuing his work, just tell me.

Please have a look at unresolved comments in the original PR.

@fgalan
Copy link
Member Author

fgalan commented Oct 7, 2020

Note old PR has diff values of +1,624 -13 while this one (at the moment of the first commit, just after porting) has +1,621 −12. The +3 -1 difference is due to a "cleaner" CHANGES_NEXT_RELEASE addition in this PR.

@Madhu-NEC
Copy link
Contributor

Hi @fgalan ,

I have started working on this PR.

Thanks!

@fgalan
Copy link
Member Author

fgalan commented Nov 4, 2020

Hi @fgalan ,

I have started working on this PR.

Thanks!

Great to know! :)

I see two options:

  1. Take the content of this PR (https://patch-diff.githubusercontent.com/raw/telefonicaid/iotagent-node-lib/pull/922.diff) to create one of yours and continue the work from it. In that case, this PR [WIP] "configGroups" literal in configuration group operations URL (old PR #780) #922 would be closed at the end.
  2. Do a PR to complete this one, using feature/752_cgroup_names as base branch. Once that PR gets merged, this one (PR [WIP] "configGroups" literal in configuration group operations URL (old PR #780) #922) would be merge with the whole work.

As you prefer... :)

@fgalan
Copy link
Member Author

fgalan commented Nov 19, 2020

(Self-note)

The PR would need some adaptation to solve the conflicts. Basically:

  1. Solve the conflict accepting your changes
  2. Change style with npm run prettier

(Please @jason-fox amend mi if I'm wrong)

@fgalan
Copy link
Member Author

fgalan commented Nov 19, 2020

The PR would need some adaptation to solve the conflicts.

Done in c9b00ef (prettier) and a1bf53f (lint).

Before using them, I thought that the prettier target was the one modifying the code to fit the style and the lint target was the one to check that the style was one (but without modifying the code). However, both modify the code. Interesting...

@Madhu-NEC
Copy link
Contributor

Hi @fgalan,

  1. Take the content of this PR (https://patch-diff.githubusercontent.com/raw/telefonicaid/iotagent-node-lib/pull/922.diff) to create one of yours and continue the work from it. In that case, this PR [WIP] "configGroups" literal in configuration group operations URL (old PR #780) #922 would be closed at the end.

I am planning to use the first option as mentioned above.

The PR would need some adaptation to solve the conflicts.

As per my understanding, to change the style of the code in my new PR, I only need to run npm run prettier. Please confirm.

@fgalan
Copy link
Member Author

fgalan commented Nov 26, 2020

As per my understanding, to change the style of the code in my new PR, I only need to run npm run prettier. Please confirm.

I think so, but @jason-fox knows that better than me and can provide better feedback :) Maybe npm run lint is also required.

@Madhu-NEC
Copy link
Contributor

Hi @fgalan , @jason-fox
I have created the new PR#954 corresponding to this PR as discussed.

@fgalan
Copy link
Member Author

fgalan commented Dec 3, 2020

Hi @fgalan , @jason-fox
I have created the new PR#954 corresponding to this PR as discussed.

Great! So I think I can close now this PR so we continue in PR #954

@fgalan fgalan closed this Dec 3, 2020
@fgalan fgalan deleted the feature/752_cgroup_names branch December 3, 2020 12:17
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