Skip to content

Add support for maintainers#1572

Merged
carolynvs merged 7 commits intogetporter:v1from
ThorstenHans:feature/maintainers
May 18, 2021
Merged

Add support for maintainers#1572
carolynvs merged 7 commits intogetporter:v1from
ThorstenHans:feature/maintainers

Conversation

@ThorstenHans
Copy link
Contributor

What does this change

This PR adds support for specifying maintainers in porter.yml.

maintainers:
- name: "John Doe"
  email: "john.doe@mail.com"
  url: "https://domain.com/a"
- name: "Jane Doe"
  url: "https://domain.com/b"
- name: "Janine Doe"
  email: "janine.doe@mail.com"
- email: "mike.doe@mail.com"
  url: "https://domain.com/c"

Generated CNAB bundle.json will contain specified maintainers.

Both - maintainers and the nested fields - are optional.

What issue does it fix

Closes #1559

Notes for the reviewer

Had to update the simple manifest digest in pkg/cnab/config-adapter/stamp_test.go.

@carolynvs carolynvs changed the base branch from main to v1 May 10, 2021 15:20
@carolynvs
Copy link
Member

I have retargeted this branch to the new v1 branch. Going forward from today (I just created it this morning), check if the issue is in the v1 milestone and if it is, create your feature branch based on the v1 branch so that you don't end up with merge conflicts because v1 and main are going to diverge quickly. 👍

@carolynvs carolynvs self-assigned this May 10, 2021
Copy link
Member

@carolynvs carolynvs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good! I just have some small questions.


}

func getMaintainerByName(source *[]bundle.Maintainer, name string) (*bundle.Maintainer, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Can you help me understand why source is a pointer here?
  • Also why is the returned maintainer entry a pointer?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pointer makes no sense for source as the slice is passed by ref anyways.

For the return value, my intention was to prevent returning a new copy.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it is a small struct (3 strings) and doesn't make use of any pointer semantics (it doesn't modify its own data), the convention in Go is to usually pass back a copy anyway. The data will be allocated on the stack (instead of the heap) and the memory recovered without having to rely on garbage collection.

It's not a huge deal but it does help avoid nil pointer concerns and is easier to work with than pointers.

@ThorstenHans ThorstenHans force-pushed the feature/maintainers branch from 6786c7d to 0783fc5 Compare May 11, 2021 08:53
@carolynvs
Copy link
Member

There is one more change to make in this PR. If you remember from the PR checklist one of the items is the schema. We have a jsonschema document for the porter.yaml. It lets us validate the porter.yaml contents and also give people autocomplete when they use editors like VS Code.

We need to update the jsonschema to include the new maintainers section. I wasn't sure if you are familiar with jsonschema so here is what the change would look like:

carolynvs@236e88a#diff-79f56eb4547c3a381285d1f26793b2cbd2e3d540e8783654511a91b4332eafd5

I have just added a magefile target that makes it easier to change the porter schema and fix the resulting tests that fail. Either rebase or merge the latest of main into your branch, and then you can then run mage updateTestfiles, it will update the tests with the new output of porter schema for you.

@ThorstenHans ThorstenHans force-pushed the feature/maintainers branch from 0783fc5 to 9aa37d1 Compare May 18, 2021 10:20
@ThorstenHans
Copy link
Contributor Author

ThorstenHans commented May 18, 2021

had to update my for to get the new target for mage. However, the commit cbd22dc (which is already on main)is not signed.

@carolynvs
Copy link
Member

Don't worry about DCO bot messages for commits that aren't yours. It was actually one of my PRs that merged funny, it only took the title and didn't keep the commit message with the signoff. I can always verify manually and mark the PR as signed. 👍

@carolynvs
Copy link
Member

Oops, when I asked you to rebase I said the wrong branch to rebase against. It should have been v1, which is why you are now seeing extra commits in your pull request. Sorry about that!

Here's how to fix it:

  1. Get the latest changes on the v1 branch
    git fetch https://github.com/getporter/porter.git v1:v1
    
  2. Rebase your branch on v1
    git checkout feature/maintainers
    git rebase -i v1
    
  3. All of the commits that the rebase command shows you should be yours now. Save the file and let the rebase continue.
  4. Now if you look at the log of your branch it should be all of your commits without the extras from main.
  5. Push your changes back to your PR with a --force push.

@carolynvs
Copy link
Member

If you run into trouble with the rebase, I'm happy to fix up the PR for you so that we can merge it.

Signed-off-by: Thorsten Hans <thorsten.hans@gmail.com>
Signed-off-by: Thorsten Hans <thorsten.hans@gmail.com>
Signed-off-by: Thorsten Hans <thorsten.hans@gmail.com>
Signed-off-by: Thorsten Hans <thorsten.hans@gmail.com>
Signed-off-by: Thorsten Hans <thorsten.hans@gmail.com>
Signed-off-by: Thorsten Hans <thorsten.hans@gmail.com>
Signed-off-by: Thorsten Hans <thorsten.hans@gmail.com>
@ThorstenHans ThorstenHans force-pushed the feature/maintainers branch from 9aa37d1 to 264c1cc Compare May 18, 2021 14:08
@ThorstenHans
Copy link
Contributor Author

there we go

Copy link
Member

@carolynvs carolynvs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks!

@carolynvs carolynvs merged commit cac46b8 into getporter:v1 May 18, 2021
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.

Add field for maintainers to porter.yaml

2 participants