Open sam-1pass opened 4 months ago
I have 2 questions:
Why do we want to run the example in the pipeline? I think we achieve the exact same coverage (and even more) with the integration tests.
If we have a good reason to introduce the example to the pipeline, why don't we just change the secret reference in the example, as opposed to creating a new service account token for that weird, general secret reference which is meant for example purposes.
I have 2 questions:
Why do we want to run the example in the pipeline? I think we achieve the exact same coverage (and even more) with the integration tests.
When I pushed changes that separated the client file into static and generated content, the pipeline erroneously passed despite the example command npm run commonjs-example
failing. This alone may be reason enough to include running the example command in the pipeline.
If we have a good reason to introduce the example to the pipeline, why don't we just change the secret reference in the example, as opposed to creating a new service account token for that weird, general secret reference which is meant for example purposes.
This is a good point. I overlooked the easier solution 😅
If we have a good reason to introduce the example to the pipeline, why don't we just change the secret reference in the example, as opposed to creating a new service account token for that weird, general secret reference which is meant for example purposes.
The example should first and foremost serve as an example, intuitive for new users of the SDK. Having an op://5edut84whnc8ut4cnw4hu8cw4/xw4cnge5nucnwhu4chnu4/e5c4wgnfgnhuewcnuw4nuh
reference in there is not intuitive. With a ~5min time investment, we can achieve both the goals of having the example be intuitive as well as validating at all times that the exports are configured correctly, for regression testing.
Should a proper test suite cover at least the same functionality as the example? If the test cases are passing and the example is failing, I think we should update the test cases. That being said, there's no harm in running the example in the pipeline as well, but it doesn't feel like a good single point of failure to rely upon as its outside the test suite.
When I pushed changes that separated the client file into static and generated content, the pipeline erroneously passed despite https://github.com/1Password/onepassword-sdk-js/pull/15#discussion_r1542466614. This alone may be reason enough to include running the example command in the pipeline.
Okay, I see, so the reasoning is in line with the bullet point I added to my comment above.
Here are some additional thoughts:
Having an op://5edut84whnc8ut4cnw4hu8cw4/xw4cnge5nucnwhu4chnu4/e5c4wgnfgnhuewcnuw4nuh reference in there is not intuitive
- Is there any reason why we can't use the actual vault and item names?
op://Test/RogerLogin/password
seems even better thanop://vault/item/field
in terms of UX.
I definitely don't think creating an additional SA token is the best solution though.
The value in adding the example as a test is twofold:
I'd like to be mindful that we don't spend more time debating what the best solution is then actually going about implementing it, since this change will nonetheless be beneficial both to the users and to us, regardless if which way we decide to go.
I am okay to use op://Test/RogerLogin/password
as a secret reference. Let's add, as a comment in the example crate, a link to the secret reference syntax: https://developer.1password.com/docs/cli/secrets-reference-syntax/ if we decide to go about it this way.
@hculea I went through this PR again.
Here is the issue I'm concerned with: as it currently stands, if we merge this PR, every future PR will run the code example for the same version beta.6. This indeed tests that beta.6 runs ok in the code example, BUT:
Add
npm run commonjs-example
command to the pipeline. This is to ensure that changes to the code don't break the example.