feat!: Add path variable support to GCS destination plugin.

[shubham-padia]

Fixes #15097 . Since the PR is fairly large, please review it commit by commit along with a read of the commit message descriptions to get a better sense of the changes.

Summary

Add path variable support to GCS destination plugin. This is a breaking change. We check if the path string has any of the template variables like {{FORMAT}}, {{TABLE}} and so on. If they do, we follow the behaviour of S3 plugin wherein we replace the template variables and that's it. If there are no such variables, we revert back to the current behaviour in the code block shown above.

Please read the issue comments to check on how the decisions for this PR were made.

This PR follows similar approach to the S3 plugin. I've added some extra tests for client.read() errors which we should probably copy back to S3 and File plugins in a follow up issue.

Note: I have not made additions to CHANGELOG.md, I'm assuming the maintainer will do it just before merge, in order to make sure of the right tag at the time of merge. Please lmk if I need to make that change

GCS screenshot for path `new/{{TABLE}}/{{UUID}}.{{FORMAT}}`

• Read the contribution guidelines 🧑‍🎓
• Run make lint to ensure the proposed changes follow the coding style 🚨 (install golangci-lint here)
• Run make test to ensure the proposed changes pass the tests 🧪
• If changing a source plugin run make gen to ensure docs are up to date 📝
• Ensure the status checks below are successful ✅

[shubham-padia]

I checked the failing runs:

1. Fixed the grammar mistake in the documentation at https://github.com/cloudquery/cloudquery/compare/693d590150666a0282a090934885d80fd34d5df9..ccbbec85f57557e6582cf6ef4ce72ad103240f7d
2. For https://github.com/cloudquery/cloudquery/actions/runs/8652900255, $ACTIONS_ID_TOKEN_REQUEST_TOKEN is not available to this PR since I think this is running from a fork?
3. For https://github.com/cloudquery/cloudquery/actions/runs/8652900268, where wait_for_required_workflows fails, it might have to do something with kodiakhq and auto merging? I could look more into it but a maintainer might be able to help me out much quicker if it's a small issue.

[erezrokah]

2. For https://github.com/cloudquery/cloudquery/actions/runs/8652900255, $ACTIONS_ID_TOKEN_REQUEST_TOKEN is not available to this PR since I think this is running from a fork?

💯 Credentials are not available for forked PR, I'll open a copy of this PR from a non forked branch to validate

3. where wait_for_required_workflows fails

Is a custom workflow we wrote to facilitate branch protection enforcement in monorepos without having to run workflows for all monorepos components due to https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/collaborating-on-repositories-with-code-quality-features/troubleshooting-required-status-checks#handling-skipped-but-required-checks

It's failing since due to the issue with the credentials

[erezrokah]

Looks great @shubham-padia, added a few minor comments, sorry for the delayed review

[erezrokah]

Suggested change

	if err != nil {
	if expectErrorWhenReading {
	assert.ErrorContains(t, err, readErrorString)
	t.Skip()
	}
	t.Fatal(fmt.Errorf("failed to sync: %w", err))
	} else if expectErrorWhenReading {
	t.Fatal(fmt.Errorf("expected error while performing client.read(), got None"))
	}
	if expectErrorWhenReading {
	require.ErrorContains(t, err,readErrorString)
	} else {
	require.NoError(t, err)
	}

Could this work instead? Regardless what's the reason we have t.Skip() in the test?

[shubham-padia]

Yes, that works much better, thanks @erezrokah. This is my first time using Go, so this is much appreciated!

I was using t.Skip() to skip the rest of the test when we encounter an error but I saw this file uses return, so I changed it to that.

[erezrokah]

OK so I think we can drop the return in both cases. That's the reason to use require over assert. require stops the test on failure https://github.com/stretchr/testify?tab=readme-ov-file#require-package

[shubham-padia]

@erezrokah I think we still need this return, require will stop the tests on failure, but failure in this case would mean either not getting an error or the error not containing the string that we provided. If the error contains the message, the require assertion would pass successfully and move on to the next part of the tests

[erezrokah]

You're right 👍 How about a25ccda to early return and reduce the if/else nesting?

I also added another commit to remove the boolean flag in 0982124

WDYT?

[shubham-padia]

Yes, much cleaner, thanks!

[erezrokah]

Looks like tests are passing https://github.com/cloudquery/cloudquery/actions/runs/8665051208/job/23762862668?pr=17608. For the changelog we generate those automatically based on the commit message.

To trigger a major version bump the PR title should start with feat!: (see https://github.com/cloudquery/cloudquery/blob/main/CONTRIBUTING.md#commit-messages)

[erezrokah]

@shubham-padia please let me know what you think of my recent changes. I think the PR is ready to be merged unless there's something I missed

[shubham-padia]

@erezrokah Those changes look good, thanks! This should be good to merge!

[erezrokah]

We'll release the new version #17611 on Tuesday April 16th as a part of our weekly cadence of releases