feature branches by shuyama1 · Pull Request #8298 · GoogleCloudPlatform/magic-modules

shuyama1 · 2023-07-10T14:57:33Z

If this PR is for Terraform, I acknowledge that I have:

Searched through the issue tracker for an open issue that this either resolves or contributes to, commented on it to claim it, and written "fixes {url}" or "part of {url}" in this PR description. If there were no relevant open issues, I opened one and commented that I would like to work on it (not necessary for very small changes).
Ensured that all new fields I added that can be set by a user appear in at least one example (for generated resources) or third_party test (for handwritten resources or update tests).
Generated Terraform providers, and ran make test and make lint in the generated providers to ensure it passes unit and linter tests.
Ran relevant acceptance tests using my own Google Cloud project and credentials (If the acceptance tests do not yet pass or you are unable to run them, please let your reviewer know).
Read Write release notes before writing my release note below.

Release Note Template for Downstream PRs (will be copied)

melinath

A few questions and change requests below.

melinath · 2023-07-11T15:55:24Z

        UPSTREAM_OWNER=GoogleCloudPlatform
        UPSTREAM_BRANCH=main
+        if [ -n "$BASE_BRANCH" ]; then
+            if [ "$BASE_BRANCH" != "main" ]; then


FWIW I think this if statement is probably not necessary since main is a valid downstream branch... I think we could always set UPSTREAM_BRANCH=$BASE_BRANCH here.

melinath · 2023-07-11T15:59:30Z

 function clone_repo() {
    SCRATCH_OWNER=modular-magician
    UPSTREAM_BRANCH=main
+    if [ -n "$BASE_BRANCH" ]; then


What would you think about checking whether BASE_BRANCH is set once at the top of the file and then setting it to main by default if unset? I think that might let you remove other checks about whether it's set or not? And then you could have UPSTREAM_BRANCH=$BASE_BRANCH as the default in this function.

That's a good idea. Moved the check to the top of each file.

melinath · 2023-07-11T16:02:33Z

+    # the check of existence of BASE_BRANCH is for backwards-compatability
+    if [ -n "$BASE_BRANCH" ]; then
+        echo "BASE_BRANCH: $BASE_BRANCH"
+        git -C $LOCAL_PATH checkout $BASE_BRANCH


Would this work? It seems like the git clone might have to happen before checking out a branch.

FWIW you might also be able to use git clone $GITHUB_PATH $LOCAL_PATH --branch $BASE_BRANCH if there's a default value for BASE_BRANCH.

Absolutely right. I checked my previous test PR, yeah, it should happen after git clone. Clearly I can't copy and paste 🤦 sorry for all these typos

melinath · 2023-07-11T16:06:57Z

@@ -5,6 +5,10 @@ set -e
 function clone_repo() {


Could you test whether the containers behave as expected, at least for generate-diffs?

I did test the generate-diff before: #7882
But I will test the change in this PR again to make sure it work as expected

shuyama1 · 2023-07-14T08:58:32Z

 echo "Checking for modified go files"
 # get the names of changed files and look for go files
 # (ignoring "no matches found" errors from grep)
-gofiles=$(git diff --name-only HEAD~1 | { grep -e "\.go$" -e "go.mod$" -e "go.sum$" || test $? = 1; })


I think this should fix the go file detection issue occurs when no diff generated for the google providers

I can't get this to run locally so I'm a little hesitant about including it. If it works in cloudbuild then that's fine for now at least, though.

I'll do a more thorough test on this feature. I'll exclude it from this PR and create a separate PR for it since I don't think feature branch is blocked without this change.

shuyama1 · 2023-07-14T16:08:12Z

The change is tested in #8351 and #8353

melinath · 2023-07-14T22:32:17Z

    elif [ "$REPO" == "terraform-google-conversion" ]; then
        UPSTREAM_OWNER=GoogleCloudPlatform
-        UPSTREAM_BRANCH=main
+        # UPSTREAM_BRANCH=main


Suggested change

# UPSTREAM_BRANCH=main

melinath · 2023-07-14T22:32:46Z

    SCRATCH_PATH=https://modular-magician:$GITHUB_TOKEN@github.com/$SCRATCH_OWNER/$GH_REPO
    mkdir -p "$(dirname $LOCAL_PATH)"
-    git clone $GITHUB_PATH $LOCAL_PATH
+    # git clone $GITHUB_PATH $LOCAL_PATH


Suggested change

# git clone $GITHUB_PATH $LOCAL_PATH

melinath · 2023-07-14T22:37:39Z

 mkdir fixtures
-gsutil -m -q cp gs://ci-vcr-cassettes/beta/fixtures/* fixtures/
-# copy branch specific cassettes over master. This might fail but that's ok if the folder doesnt exist
+if [ "$BASE_BRANCH" != "FEATURE-BRANCH-major-release-5.0.0" ]; then


In the future it would be nice to use a more generic matching rule like "does not start with FEATURE-BRANCH-major-release" but this is fine for now and not worth delaying to change probably.

Let's use FEATURE-BRANCH-major-release-5.0.0 for now and I'll change it to be more generic after we confirm this path working fine in the future.

melinath · 2023-07-14T22:55:53Z

          - -c
-          - git push https://modular-magician:$$GITHUB_TOKEN@github.com/GoogleCloudPlatform/magic-modules $COMMIT_SHA:tpg-sync
+          - |
+            if [ "$BRANCH_NAME" == "main" ]; then


I wonder if using the substitution directly like this might have security issues similar to https://docs.github.com/en/actions/security-guides/security-hardening-for-github-actions#using-an-intermediate-environment-variable? It doesn't seem to be documented as an issue on https://cloud.google.com/build/docs/configuring-builds/run-bash-scripts but they do recommend using script with env vars now rather than using bash + args. (But that could just be because it was confusing users to have to sort out what is a substitution vs an env var.)

In any case - I don't think this is an issue since allowed branch names are pretty constrained by git.

melinath · 2023-07-14T23:08:48Z

 echo "Checking for modified go files"
 # get the names of changed files and look for go files
 # (ignoring "no matches found" errors from grep)
-gofiles=$(git diff --name-only HEAD~1 | { grep -e "\.go$" -e "go.mod$" -e "go.sum$" || test $? = 1; })


I can't get this to run locally so I'm a little hesitant about including it. If it works in cloudbuild then that's fine for now at least, though.

shuyama1 requested a review from melinath July 10, 2023 15:23

melinath requested changes Jul 11, 2023

View reviewed changes

shuyama1 force-pushed the feature_branch branch from b8fba0f to cbc0ea1 Compare July 13, 2023 23:20

shuyama1 commented Jul 14, 2023

View reviewed changes

shuyama1 requested a review from melinath July 14, 2023 08:58

melinath approved these changes Jul 14, 2023

View reviewed changes

shuyama1 added 11 commits July 19, 2023 11:47

feature branch

d787409

update

ea74f56

update

66ad637

update

27cf020

update

c291917

update

37744b4

fix downstream pr info

ff16a7a

clean up

33e5c4a

protential fix for go file change detection

2d49e1c

fix vcr_merge

9564f22

clean up

7876900

shuyama1 force-pushed the feature_branch branch from ab0e829 to 7876900 Compare July 19, 2023 18:48

shuyama1 merged commit 4713a03 into GoogleCloudPlatform:main Jul 19, 2023

shuyama1 deleted the feature_branch branch July 19, 2023 19:03

modular-magician mentioned this pull request Jul 19, 2023

feature branches GoogleCloudPlatform/terraform-google-conversion#1178

Merged

5 tasks

melinath pushed a commit that referenced this pull request Jul 20, 2023

feature branches (#8298)

85a04ae

ericayyliu pushed a commit to ericayyliu/magic-modules that referenced this pull request Jul 26, 2023

feature branches (GoogleCloudPlatform#8298)

d6ef394

wj-chen pushed a commit to wj-chen/magic-modules that referenced this pull request Aug 1, 2023

feature branches (GoogleCloudPlatform#8298)

5a2e125

DanielRieske pushed a commit to bschaatsbergen/magic-modules that referenced this pull request Aug 2, 2023

feature branches (GoogleCloudPlatform#8298)

1c6ea3c

hao-nan-li pushed a commit to hao-nan-li/magic-modules that referenced this pull request Aug 9, 2023

feature branches (GoogleCloudPlatform#8298)

eef384d

Uh oh!

Conversation

shuyama1 commented Jul 10, 2023

Uh oh!

melinath left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shuyama1 commented Jul 14, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants