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

feat(internal/carver): add tooling to help carve out sub-modules #4417

Merged
merged 9 commits into from
Jul 26, 2021
Merged

Conversation

codyoss
Copy link
Member

@codyoss codyoss commented Jul 12, 2021

No description provided.

@codyoss
Copy link
Member Author

codyoss commented Jul 12, 2021

Here is an example PR that was raised the code: #4419

Steps:

  1. Ran:
go run cmd/main.go \
  -parent=/path/to/google-cloud-go \
  -child=asset \
  -repo-metadata=/path/to/google-cloud-go/internal/.repo-metadata-full.json
  1. Ran git push remote branch.
  2. After PR is merged run: git push remote --tags

@codyoss codyoss marked this pull request as ready for review July 12, 2021 19:53
@codyoss codyoss requested a review from a team as a code owner July 12, 2021 19:53
@codyoss codyoss requested review from tbpg and broady July 12, 2021 19:53
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jul 13, 2021
internal/carver/README.md Outdated Show resolved Hide resolved
internal/carver/cmd/_CHANGES.md.txt Outdated Show resolved Hide resolved
internal/carver/cmd/_README.md.txt Outdated Show resolved Hide resolved
internal/carver/cmd/_README.md.txt Outdated Show resolved Hide resolved
internal/carver/cmd/_README.md.txt Outdated Show resolved Hide resolved
internal/carver/cmd/main.go Outdated Show resolved Hide resolved
}
childPrefix := strings.TrimPrefix(strings.TrimPrefix(c.childModPath, rootMod.filePath), "/")
log.Println("Commiting changes")
if !c.dryRun {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should dry run print the things it would do if it weren't a dry run?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not sure I understand. Are you suggestioning to print more or less when it is a dry run? Right now it does some light printing. I had added this originally to test to see what tags were chosen which I thought was useful.

// sortTags does a best effort sort based on semver. It was made a function for
// testing. Only the top result will ever be used.
func sortTags(tags []string) {
sort.Slice(tags, func(i, j int) bool {
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
Member Author

Choose a reason for hiding this comment

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

I was unaware of this package, thanks I will try this out!

Copy link
Member Author

Choose a reason for hiding this comment

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

After trying this out it does sort by semver but not in the way I needed. The main difference between the two results is that I derank pre-releases a whole major version as we don't want to find those tags when picking a base tag to work off of.

})
}

func parsePkgName(importPath string) (string, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is more like guessPkgName since the name may not always be the final element of the import path. Is there a better way?

Copy link
Member Author

Choose a reason for hiding this comment

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

I renamed the variable. This is actually the child modules filepath. Which I believe the last element will always be the name of the package found at that level. Can you think of a case where this is not true?

if name == "" {
name = meta[importPath]
if name == "" {
return fmt.Errorf("unable to determine a name from API metadata, please set -name flag")
Copy link
Contributor

Choose a reason for hiding this comment

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

Give path to metadata file? And the import path that was being looked for?

Copy link
Member Author

Choose a reason for hiding this comment

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

In this case metadata file exists but does not contain the right info to split out the module. I think there are some cases where we might have a structure of:

/
   /baz
      /foo
         /apiv1
      /bar
         /apiv2

If we specified baz as the level to split the module out the metadata lookup fails.

codyoss and others added 5 commits July 16, 2021 08:11
Co-authored-by: Tyler Bui-Palsulich <26876514+tbpg@users.noreply.github.com>
Co-authored-by: Tyler Bui-Palsulich <26876514+tbpg@users.noreply.github.com>
Co-authored-by: Tyler Bui-Palsulich <26876514+tbpg@users.noreply.github.com>
@codyoss codyoss requested a review from tbpg July 22, 2021 16:36
@codyoss codyoss added the automerge Merge the pull request once unit tests and other checks pass. label Jul 26, 2021
@gcf-merge-on-green gcf-merge-on-green bot merged commit a7e28f2 into googleapis:master Jul 26, 2021
@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Jul 26, 2021
@codyoss codyoss deleted the carver branch September 11, 2021 19:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants