fix(sdk-rtl/src/delimArray): Fixed issue with single element number array

[akash-looker]

Issue

DelimArray internally extends Array provided by javascript.
The issue of initialising an array via Array constructor with single number argument is that the newly created array will be of length specified as the argument, with actual value as undefined unlike when we pass array with more than one values (MDN reference).

const singleValuedArray = new Array(2)
console.log(singleValuedArray.length) // 2
console.log(singleValuedArray[0]) // undefined
console.log(singleValuedArray[1]) // undefined

const multipleValuedArray = new Array(1,2)
console.log(multipleValuedArray.length) // 2
console.log(multipleValuedArray[0]) // 1
console.log(multipleValuedArray[1]) // 2

Since DelimArray internally initialises array via Array constructor, we get unexpected output with array of single number.

const delimitedSingleValuedArray = new DelimArray<number>([2]);
console.log(delimitedSingleValuedArray) // actual output: ',' ; expected output: 2

const delimitedMultipleValuedArray = new DelimArray<number>([1,2]);
console.log(delimitedMultipleValuedArray) // actual / expected output: '1,2'

Fix

Instead of creating an array with the values during the initialisation step, we can use Array.prototype.push method to populate the aforementioned array object with the values passed to the DelimArray in the initialisation step.

Developer Checklist ℹ️

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

• Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
• Ensure the tests and linter pass
• Appropriate docs were updated (if necessary)

Fixes #<issue_number_goes_here> 🦕

[joeldodge79]

looks good to me, nice catch!

[joeldodge79]

@drstrangelooker I think we're hitting the issue where the on: pull_request event doesn't give secrets to forked PRs. Not sure what the solution is here - we'd like to encourage PRs from forks but we can't use pull_request_target because we do a source code checkout.

It would be really nice if GitHub offered the option for a maintainer to "run Actions with secrets" for a PR - or maybe the UI at the top of this article can be limited to certain trusted forks?

I think maybe there needs to be an intermediate step of a forked PR is created against a non-main branch and then a maintainer merges that in and runs CI (after, of course, verifying there's no secret-exfiltrating or other malicious code). Then merging that branch into main when it passes. The only concern would be whether the original authorship is still attributed correctly.

I'm gonna let DevRel drive that.

WARNING: Could not open the configuration file: [/home/runner/.config/gcloud/configurations/config_default].
Error response from daemon: Head "https://us-west1-docker.pkg.dev/v2/cloud-looker-sdk-codegen-cicd/looker/21_16/manifests/latest": denied: Permission "artifactregistry.repositories.downloadArtifacts" denied on resource "projects/cloud-looker-sdk-codegen-cicd/locations/us-west1/repositories/looker" (or it may not exist)

[joeldodge79]

also, another side effect of running a PR from a fork is that the publish-test-results job we rely on as a "required check" logs this and passes instead of failing:

2021-12-20 15:33:38 +0000 - github_action - WARNING - This action is running on a pull_request event for a fork repository. It cannot do anything useful like creating check runs or pull request comments.
Warning: This action is running on a pull_request event for a fork repository. It cannot do anything useful like creating check runs or pull request comments.

[jkaster]

Good find, good fix

[akash-looker]

Based on the all the info added here and offline discussion with @joeldodge79, the failing jobs aren't related to the changes in this PR. Go going ahead with the merge.

Thanks @joeldodge79 @jkaster for the review.