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

Per project configuration #27

Closed
spalger opened this issue Nov 30, 2017 · 14 comments
Closed

Per project configuration #27

spalger opened this issue Nov 30, 2017 · 14 comments

Comments

@spalger
Copy link
Contributor

spalger commented Nov 30, 2017

The config file currently lives in ~/.backport, which is fine, but it would be awesome if we could check-in the config for a project (like the repo location, branches, etc)

@sorenlouv
Copy link
Owner

sorenlouv commented Nov 30, 2017

I imagine the config would look something like:

{
    "name": "elastic/kibana",
    "versions": ["6.x", "6.1", "6.0"],
    "labels": ["backport"]
}

Is that what you had in mind? It will only be picked up, if backport command is run from inside the repo folder, eg elastic/kibana/plugins. What should happen if a project config is picked up, and the user-defined config also specifies branches for the same project? Should they be merged? And which should take precedence?

@spalger
Copy link
Contributor Author

spalger commented Nov 30, 2017

I would probably change the config resolution to function more like:

  1. look for a .backportrc file starting in cwd and walking up (using find-up or something)
  2. if no .backportrc file is found, fail
  3. look for a ~/.backport/config.json file
  4. if no ~/.backport/config.json is found, create config file and fail

~/.backport/config.json should look like:

{
  "accessToken": "...",
  "username": "..."
}

.backportrc.json should look like this:

{
  "upstream": "elastic/kibana",
  "origin": "{username}/kibana",
  "own": true,
  "multiple": true,
  "versions": ["6.x", "6.0", "5.6", "5.5", "5.4"],
  "labels": ["backport"]
}

I don't think there's any reason the backport tool needs to support overrides or per-project config in the ~/.backport/config.json file, if they want per-project configurations that should just add a .backportrc file to their project. They don't have to commit it if they are just trying things out.

@sorenlouv
Copy link
Owner

sorenlouv commented Nov 30, 2017

That makes sense! I also prefer the naming you use, eg. upstream.

A couple of thoughts:

  • Currently origin is derived from upstream (replaces "elastic" with {username}) - so is origin necessary to have explicitly?
  • multiple has been replaced with multipleCommits and multipleVersions. I feel it would be hard for all devs in a project to converge on the same settings. Eg. I always backport one commit at a time (multipleCommits: false) but backport to several versions (multipleVersions: true). Others might do it differently. With multipleCommits: true you can obviously still decide just to select 1 commit, but it requires an extra click (spacebar, then enter).

That's just my impression of how people work but it would be great if we can have less user-specific options, and users don't have to think.

@sorenlouv
Copy link
Owner

sorenlouv commented Nov 30, 2017

Maybe ~/.backport/config.json can override whatever project-specific settings we have, but by default it only contains:

{
  "accessToken": "...",
  "username": "..."
}

as you said. That way users actively have to override the project settings, if they really want to, but it'll Just Work for them if they don't.

@sorenlouv
Copy link
Owner

I don't think there's any reason the backport tool needs to support overrides or per-project config in the ~/.backport/config.json file, if they want per-project configurations that should just add a .backportrc file to their project

My thinking is: if I add .backportrc to Kibana, and someone doesn't like the settings, shouldn't they be able to change them for themselves?

@epixa
Copy link

epixa commented Dec 1, 2017

Personally, I think if a project specifically wants certain configurations, individual users shouldn't be able to override them. Otherwise we can't reliably ensure that our automation is doing what we need/want it to do.

@spalger
Copy link
Contributor Author

spalger commented Dec 1, 2017

someone doesn't like the settings, shouldn't they be able to change them for themselves?

@sqren I assume you're thinking about options like "own" and "multiple". I think it makes sense to limit those to CLI flags and maybe make them global (not per project) in .backport/config.json.

I guess part of my design includes not specifying project-specific configuration in the .backport/config.json file because I'm not super fond of the way configs are mapped to the "current project". If I'm reading the implementation correctly it currently tests that basename(process.cwd() matches the last path segment of repositories[i].name, which surprised me. By removing all project specific config from there we can remove that mapping and just rely on properly placed config files (which I think most users are used to).

@sorenlouv
Copy link
Owner

If I'm reading the implementation correctly it currently tests that basename(process.cwd() matches the last path segment of repositories[i].name, which surprised me

Yeah, we definitely want to get rid of that. That should also happen automatically with the addition of a .backportrc.json.

I assume you're thinking about options like "own" and "multiple". I think it makes sense to limit those to CLI flags and maybe make them global (not per project) in .backport/config.json.

Yes, it was exactly those I meant. I'll try removing all the project specific stuff, and move it to a .backportrc.json - that's also more in line with other projects.

@sorenlouv
Copy link
Owner

sorenlouv commented Dec 1, 2017

@spalger I just talked to @vanjacosic and he had a good point about backport labels. Currently his config looks like:

{
  "repositories": [
    {
      "name": "elastic/kibana",
      "versions": ["6.x", "6.1"],
      "labels": ["apm", "backport"]
    }
  ]
}

As I understand your suggestion, he would only be able to add custom labels (like "apm")

  • if .backportrc.json is not checked in (and thus is per user)
  • if he can override it from a global config (~/.backport/config.json)

Do you see any good ways to support this use-case?

@epixa
Copy link

epixa commented Dec 1, 2017

As far as I know he's the only person doing this - is it necessary? The backports link to a canonical issue that has the relevant feature/team labels on it. The backport label itself is really just there for filtering and automation.

@spalger
Copy link
Contributor Author

spalger commented Dec 1, 2017

"labels": ["apm", "backport"]

That's an interesting idea, and seems like a reasonable feature to provide. Maybe the global config could accept an array of labels and apply those without filtering by project, or perhaps we could change the per-project overrides in the global config to be selected with a path of some sort rather than the current algo.

Something like this perhaps:

{
  "overrides": [
    {
      // must point to the directory containing a .backportrc file
      "project": "~/dev/kibana",
      "labels": ["apm", "backport"]
    }
  ]
}

Alternatively, we could add --labels apm,backport support to the CLI (might already support it) and people could setup bash aliases for custom config.

@sorenlouv
Copy link
Owner

sorenlouv commented Dec 3, 2017

@spalger @epixa
I ended up with a compromise, so the project config (.backportrc.json) behaves as described in #27 (comment), but can be overridden by the global config (.backport/config.json). By default the global config only contains the required fields (accessToken and username), so most users will never override the project specific settings like labels - I see it as a hidden feature for power users.

There is an open PR #29 and I've published a beta version that you can install if you want to try it out: npm install backport@2.1.1-beta

With the beta you can add the following .backportrc.json to the kibana repo:

{
  "upstream": "elastic/kibana",
  "versions": ["6.x", "6.1", "6.0", "5.6", "5.5", "5.4"],

  // optional
  "labels": ["backport"],
  "own": true,
  "multipleCommits": false,
  "multipleVersions": true
}

and simplify your global config ~/.backport/config.json:

{
  "accessToken": "...",
  "username": "..."
}

@sorenlouv
Copy link
Owner

Btw. I'm thinking about renaming "versions" to "branches", since that's really what they are. Any objections?

@sorenlouv
Copy link
Owner

Closed by #29

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

No branches or pull requests

3 participants