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.

Sign up for GitHub

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

Jump to bottom

IA-1790 add list to GoogleTopicAdmin #377

Merged
merged 3 commits into from
Nov 25, 2020
Merged

IA-1790 add list to GoogleTopicAdmin #377

merged 3 commits into from
Nov 25, 2020

Conversation

Qi77Qi
Copy link
Collaborator

@Qi77Qi Qi77Qi commented Nov 23, 2020 •
edited
Loading

https://broadworkbench.atlassian.net/browse/IA-1790

Add steward config to control PR frequency

PR checklist

  • For each library you've modified here, decide whether it requires a major, minor, or no version bump. (Click here for guidance)

If you're doing a major or minor version bump to any libraries:

  • Bump the version in project/Settings.scala createVersion()
  • Update CHANGELOG.md for those libraries
  • I promise I used @deprecated instead of deleting code where possible

In all cases:

  • Replace the appropriate version hashes in README.md and the CHANGELOG.md for any libs you changed with TRAVIS-REPLACE-ME to auto-update the version string
  • Get two thumbsworth of PR review
  • Verify all tests go green (CI and coverage tests)
  • Squash commits and merge to develop
  • Delete branch after merge

@Qi77Qi Qi77Qi changed the title add list to GoogleTopicAdmin IA-1790 add list to GoogleTopicAdmin Nov 23, 2020
@Qi77Qi Qi77Qi mentioned this pull request Nov 24, 2020
Merged
kyuksel
kyuksel reviewed Nov 24, 2020
View reviewed changes
Copy link
Contributor

@kyuksel kyuksel left a comment

Choose a reason for hiding this comment

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

Looks good overall. A few questions/comments/suggestions inline.

.scala-steward.conf
# If set to true, Scala Steward will attempt to update the scala version
# Since this feature is experimental, the default is set to false
# Default: false
updates.includeScala = true
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really want Steward to PR Scala version updates?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it'd be helpful. we can always revert if it becomes too much

Copy link
Contributor

Choose a reason for hiding this comment

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

okay

google2/src/main/scala/org/broadinstitute/dsde/workbench/google2/GoogleSubscriptionAdmin.scala Outdated Show resolved Hide resolved
.scala-steward.conf Show resolved Hide resolved
.scala-steward.conf
Comment on lines +38 to +39
# The dependencies which match the given version pattern are updated.
# Dependencies that are not listed will be updated.
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this comment correspond to?

Copy link
Collaborator Author

@Qi77Qi Qi77Qi Nov 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

this file is copied from https://github.com/scala-steward-org/scala-steward/blob/04b9d423d4830f1fd86d680ecef2d6372c8ec937/docs/repo-specific-configuration.md.... I think the line is explaining about the attribute below, no?

Copy link
Contributor

Choose a reason for hiding this comment

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

ah I see - the empty line (37) threw me off

.scala-steward.conf
# Default: @asap
#
#pullRequests.frequency = "0 0 ? * 3" # every thursday on midnight
pullRequests.frequency = "15 days"
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean we'll get PRs every 15 days? If so, I wonder if that'd increase the number of PRs we'll get at once, which may not be desirable since they may launch several integration test runs. It'd be nice to tell Steward to make a single PR with all version changes from the past n days but I couldn't see anything like that in the config.

Copy link
Collaborator Author

@Qi77Qi Qi77Qi Nov 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

this is wb-libs, hence no automation test involved...I think it's nice to have PRs at once so that someone can dedicate some time go through them all, and do a git shortlog --no-merges hash1..hash2 to collect all bumps in changelog

Copy link
Contributor

Choose a reason for hiding this comment

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

ah right 👍

kyuksel
kyuksel approved these changes Nov 24, 2020
View reviewed changes
Copy link
Contributor

@kyuksel kyuksel left a comment

Choose a reason for hiding this comment

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

👍 with one Scala Steward config line commenting question inline

.scala-steward.conf
# Default: @asap
#
#pullRequests.frequency = "0 0 ? * 3" # every thursday on midnight
pullRequests.frequency = "15 days"
Copy link
Contributor

Choose a reason for hiding this comment

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

ah right 👍

.scala-steward.conf Outdated
#
# Each pattern must have `groupId`, and may have `artifactId` and `version`.
# Defaults to empty `[]` which mean Scala Steward will update all dependencies.
//updates.allow = [ { groupId = "com.example" } ]
Copy link
Contributor

Choose a reason for hiding this comment

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

Is commenting supposed to be via # instead of //?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ooops...thanks!

.scala-steward.conf
Comment on lines +38 to +39
# The dependencies which match the given version pattern are updated.
# Dependencies that are not listed will be updated.
Copy link
Contributor

Choose a reason for hiding this comment

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

ah I see - the empty line (37) threw me off

.scala-steward.conf
# If set to true, Scala Steward will attempt to update the scala version
# Since this feature is experimental, the default is set to false
# Default: false
updates.includeScala = true
Copy link
Contributor

Choose a reason for hiding this comment

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

okay

@Qi77Qi Qi77Qi force-pushed the topicAdmin branch 3 times, most recently from 8620ec5 to 95e4a7a Compare November 24, 2020 20:09
@Qi77Qi Qi77Qi mentioned this pull request Nov 24, 2020
Closed
@Qi77Qi Qi77Qi force-pushed the topicAdmin branch 3 times, most recently from b7c9eb9 to 8db0ebd Compare November 25, 2020 15:06
@Qi77Qi
add list to GoogleTopicAdmin
cc7644e
@Qi77Qi
fix
d3e762d 
add subscription

fix doc

add scala-steward config

comment and fix ci

update readme

scalafmt

update changelog

update io.kubernetes client-java

update changelog
@Qi77Qi
update workflow
ca74970
@codecov
Copy link

codecov bot commented Nov 25, 2020

Codecov Report

❗ No coverage uploaded for pull request base (develop@6aa6ad9). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             develop     #377   +/-   ##
==========================================
  Coverage           ?   22.68%           
==========================================
  Files              ?       76           
  Lines              ?     1772           
  Branches           ?       38           
==========================================
  Hits               ?      402           
  Misses             ?     1370           
  Partials           ?        0

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6aa6ad9...ca74970. Read the comment docs.

@Qi77Qi Qi77Qi merged commit 13fdba5 into develop Nov 25, 2020
@Qi77Qi Qi77Qi deleted the topicAdmin branch November 25, 2020 15:43
@Qi77Qi Qi77Qi mentioned this pull request Nov 30, 2020
Merged
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kyuksel kyuksel kyuksel approved these changes

@rtitle rtitle Awaiting requested review from rtitle

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@Qi77Qi @kyuksel