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

GroupId & packages -> com.expediagroup. Docker hub org -> expediagrou… #175

Merged
merged 5 commits into from
May 3, 2019

Conversation

nahguam
Copy link
Contributor

@nahguam nahguam commented May 2, 2019

…p. Addresses #162

stream-registry PR

Changed

  • Changed groupId to com.expediagroup.streamplatform
  • Changed packages to com.expediagroup.streamplatform.streamregistry

PR Checklist Forms

  • CHANGELOG.md updated
  • Reviewer assigned
  • PR assigned (presumably to submitter)
  • Labels added (enhancement, bug, documentation)

@nahguam nahguam requested review from neoword, teabot and a team and removed request for teabot and neoword May 2, 2019 10:19
.travis.yml Show resolved Hide resolved
.mvn/wrapper/maven-wrapper.properties Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
LICENSE.txt Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
core/pom.xml Outdated Show resolved Hide resolved
core/src/test/resources/logback-test.xml Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
Copy link
Contributor

@neoword neoword left a comment

Choose a reason for hiding this comment

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

LGTM... I will setup a MIGRATION.md file in eg-master to track major items that will be necessary for HA team when they migrate.

To the degree possible when PRs are being merged in eg-master it would be nice (not required, but nice) to update the list with the PR and any necessary actions.

@neoword neoword added the enhancement New feature or request label May 3, 2019
@neoword neoword added this to In progress in Kanban Board via automation May 3, 2019
@neoword neoword added this to the eg-branch-snapshot milestone May 3, 2019
@neoword neoword merged commit 088a1a7 into eg-master May 3, 2019
Kanban Board automation moved this from In progress to Done May 3, 2019
@neoword neoword deleted the expediagroup-groupId branch May 3, 2019 21:44
@neoword
Copy link
Contributor

neoword commented May 3, 2019

See #177

@@ -1,6 +1,6 @@
#
# Copyright HomeAway, Inc 2018-Present. All Rights Reserved.
# Copyright Expedia, Inc 2018-2019. All Rights Reserved.

Choose a reason for hiding this comment

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

Sorry but this actually needs to be like this:

Copyright (C) 2018-2019 Expedia, Inc.

We should definitely do a stripped down eg-oss-parent POM which has all this stuff in it but for now maybe a small new PR with this change ^?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, yes. Please submit new PR with this change.
Good catch @massdosage

/* Copyright (c) 2018-Present Expedia Group.
* All rights reserved. http://www.homeaway.com

/* Copyright (C) 2018-2019 Expedia, Inc.
Copy link

@massdosage massdosage May 7, 2019

Choose a reason for hiding this comment

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

Ah, this is correct, so maybe just an issue with that .properties file above? I assume that one is done manually not by the plugin or something.

Copy link
Contributor

Choose a reason for hiding this comment

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

yep

## Legal
This project is available under the [Apache 2.0 License](http://www.apache.org/licenses/LICENSE-2.0.html).

Copyright 2016-2019 Expedia, Inc.

Choose a reason for hiding this comment

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

2018 is the inception year for this project not 2016 right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. This should be 2018... and we should probably update the text to be
2018-Present

Choose a reason for hiding this comment

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

I don't think the lawyers like "Present" as it doesn't have a concrete meaning and is incorrect if the project isn't changed for example next year. We should use the date that the project was last updated as the end year. The license plugin will handle it to make the changes everywhere when a new year begins and a commit is made at that point.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link

@massdosage massdosage left a comment

Choose a reason for hiding this comment

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

A few small nits, rest looks good.

@neoword - thanks for reviewing. Our convention up to now is that we require two reviewers to approve before a merge can be done and that the person who raised the PR does the merge (as they might be working on related PRs and know best what order to do them in etc.) Up to this team to decide if they want to adopt this convention or something else.

@neoword
Copy link
Contributor

neoword commented May 7, 2019

  • I like two reviewers for acceptance (I'm sure there are exceptions, right?), will do so from now on.
  • The person raising the PR may not (usually won't?) have access to commit, therefore I propose that if the acceptance criteria has been achieved (see previous point or whatever it is), that ANY committer can merge (where committer is anyone who has commit access to the branch in question).

@massdosage
Copy link

In our experience the people actively working on the project are the ones also raising the PRs and reviewing each other's work so they usually do have commit rights. I know there are times when I want to get the acceptance done but don't want it merged until I'm ready with some other thing. Depends on the PR, one can go with either approach and then the person raising the PR can put a comment like "please don't merge yet" etc. I'd say let the team of main committers on this choose what they want to do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Kanban Board
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants