Adding acceptance tests for nodejs Stack

pacostas commented 7 months ago

Merge After

https://github.com/paketo-community/ubi-nodejs-extension/pull/76 and a new ubi-nodejs-extension release is out.

Related issues

https://github.com/paketo-community/ubi-base-stack/issues/2

Summary

This PR adds acceptance tests for Node stacks. That way we ensure before each release, that stacks work properly with ubi-nodejs-extension.

Use Cases

Checklist

[x] I have viewed, signed, and submitted the Contributor License Agreement.
[x] I have linked issue(s) that this PR should close using keywords or the Github UI (See docs)
[x] I have added an integration test, if necessary.
[x] I have reviewed the styleguide for guidance on my code quality.
[x] I'm happy with the commit history on this PR (I have rebased/squashed as needed).

mhdawson commented 7 months ago

Is there a reason to move buildpack_integration_test.go into the subdirectory. Just asking as it makes it less consistent with other stacks, for example - https://github.com/paketo-buildpacks/jammy-full-stack

mhdawson commented 7 months ago

@pacostas Can you confirm that the updates in scripts/.util/tools.sh are just to bring it up to date with the template shared version of that?

mhdawson commented 7 months ago

I think keep in the simple go test might also make sense. If we ever have to temparily disable the Node.js tests it would still be good to have that basic test in place.

pacostas commented 7 months ago

Is there a reason to move buildpack_integration_test.go into the subdirectory. Just asking as it makes it less consistent with other stacks, for example - https://github.com/paketo-buildpacks/jammy-full-stack

@mhdawson They named the file buildpack_integration_test.go and I thought why not putting it under the integration folder following similar pattern to what we have on the buildpacks. BTW I should have remove string integration from the name nodejs_stack_integration_test.go as the file is moved under the integration directory. Usually on the top level they put the unit tests. The correct I think is to have all the integration tests under the integration directory. In case we put the file under the root directory, we should also move the init_test.go on the root directory. What do you think?

pacostas commented 7 months ago

@pacostas Can you confirm that the updates in scripts/.util/tools.sh are just to bring it up to date with the template shared version of that?

@mhdawson Its not fully aligned yet. I'm working on it.

Status updated: @mhdawson now its fully aligned.

mhdawson commented 7 months ago

Is there a reason to move buildpack_integration_test.go into the subdirectory. Just asking as it makes it less consistent with other stacks, for example - https://github.com/paketo-buildpacks/jammy-full-stack

@mhdawson They named the file buildpack_integration_test.go and I thought why not putting it under the integration folder following similar pattern to what we have on the buildpacks. BTW I should have remove string integration from the name nodejs_stack_integration_test.go as the file is moved under the integration directory. Usually on the top level they put the unit tests. The correct I think is to have all the integration tests under the integration directory. In case we put the file under the root directory, we should also move the init_test.go on the root directory. What do you think?

My main concern is that we can share as much as possible with the generic files like test.sh as possible. If they run something called X then ideally we should have something called X instead of a differently located/named file. Having said that, if we have X and it calls runs/tests that are in other files that is good as well. In this case I thing other stacks had a file called buildpack_integration_test.go so wanted to be consitent with the other stacks.

pacostas commented 7 months ago

Is there a reason to move buildpack_integration_test.go into the subdirectory. Just asking as it makes it less consistent with other stacks, for example - https://github.com/paketo-buildpacks/jammy-full-stack

@mhdawson They named the file buildpack_integration_test.go and I thought why not putting it under the integration folder following similar pattern to what we have on the buildpacks. BTW I should have remove string integration from the name nodejs_stack_integration_test.go as the file is moved under the integration directory. Usually on the top level they put the unit tests. The correct I think is to have all the integration tests under the integration directory. In case we put the file under the root directory, we should also move the init_test.go on the root directory. What do you think?

My main concern is that we can share as much as possible with the generic files like test.sh as possible. If they run something called X then ideally we should have something called X instead of a differently located/named file. Having said that, if we have X and it calls runs/tests that are in other files that is good as well. In this case I thing other stacks had a file called buildpack_integration_test.go so wanted to be consitent with the other stacks.

Aligned also with what exists on jammy implementation, in terms of file structure.

pacostas commented 7 months ago

@mhdawson Ive finished with the review fixes, I think I've covered all suggestions. Can you have a fresh look and see if something is missing, or if the structure is consistent enough?

pacostas commented 7 months ago

Related issue to close after merging this PR https://github.com/paketo-community/ubi-base-stack/issues/2

pacostas commented 7 months ago

@mhdawson I also added a .github/dependabot.yml file to update the go modules as they were outdated. This file is missing as is not also on the jammy-base-stack, although is present on bionic-base-stack (deprecated stack) I dont see the reason why not upgrading the go modules, at first i thought that we might have releases very often as new prs for upgrading the go modules will land, but this is not the case as we only have a release when the image hash codes change.

mhdawson commented 7 months ago

@mhdawson I also added a .github/dependabot.yml file to update the go modules as they were outdated. This file is missing as is not also on the jammy-base-stack, although is present on bionic-base-stack (deprecated stack) I dont see the reason why not upgrading the go modules, at first i thought that we might have releases very often as new prs for upgrading the go modules will land, but this is not the case as we only have a release when the image hash codes change.

Thanks, just sounds like an oversight on my part.

mhdawson commented 7 months ago

@pacostas it still seems to differ in the top level name of the integration test. In the jammy base stack:buildpack_integration_test.go

verus being changed to nodejs_stack_integration_test.go in this PR

mhdawson commented 7 months ago

I'm not sure removing the npm buildpack is good. We likely want to be testing that our run images include both the node and npm binaries. You ready build a buildpack, what would the issue being to just have it include the top level Node.js buildpack and the modified extention (to have the right run images) and use that to build/run the application, making sure that npm is used? I think in that case we would not need the build plan at all ?

mhdawson commented 7 months ago

@pacostas did another run through and left a few more comments

pacostas commented 7 months ago

@pacostas it still seems to differ in the top level name of the integration test. In the jammy base stack:buildpack_integration_test.go

verus being changed to nodejs_stack_integration_test.go in this PR

Yes, I did that intentionally to separate the nodejs stacks from the java stacks or any other stacks that are going to be added in the future, I thought is nice to have them in a separate file. Also I thought the name is little bit misleading as we are not testing buildpacks nor extensions but stacks. This doesnt affect how the integration tests are being called, as they are under the same namespace package acceptance_test. We can also sync about that on todays' meeting.

pacostas commented 7 months ago

I'm not sure removing the npm buildpack is good. We likely want to be testing that our run images include both the node and npm binaries. You ready build a buildpack, what would the issue being to just have it include the top level Node.js buildpack and the modified extention (to have the right run images) and use that to build/run the application, making sure that npm is used? I think in that case we would not need the build plan at all ?

I think is better to sync on that on todays' call, as for some reason the extension does not work without the build plan.

mhdawson commented 7 months ago

@pacostas it still seems to differ in the top level name of the integration test. In the jammy base stack:buildpack_integration_test.go

verus being changed to nodejs_stack_integration_test.go in this PR

Ok understand based on discussion today, tests are run based on package versus the name so naming this way is good.

pacostas commented 7 months ago

I'm not sure removing the npm buildpack is good. We likely want to be testing that our run images include both the node and npm binaries. You ready build a buildpack, what would the issue being to just have it include the top level Node.js buildpack and the modified extention (to have the right run images) and use that to build/run the application, making sure that npm is used? I think in that case we would not need the build plan at all ?

I think is better to sync on that on todays' call, as for some reason the extension does not work without the build plan.

Based on todays discussion, I'll add the buildpacks we usually use to build an app, as we dont want to have unexpected errors coming from extension/buildpack combinations that we are not fully aware.

sophiewigmore commented 6 months ago

Hey @pacostas, let me know when you'd like a PR review on this. I haven't been sure of the status

pacostas commented 6 months ago

@sophiewigmore Yes, this is ready, the error on the acceptance tests has to do with the network, so re-running the tests probably everything will be fine :)

sophiewigmore commented 6 months ago

Unfortunately, it looks like the tests are failing pretty consistently here :/

pacostas commented 6 months ago

Unfortunately, it looks like the tests are failing pretty consistently here :/

@sophiewigmore Apologies, this comment was meant for the other PR https://github.com/paketo-community/ubi-base-stack/pull/41 :S ( I wrote in wrong browser tab)

You are right, this PR wont pass, as it needs to be merged after https://github.com/paketo-community/ubi-nodejs-extension/pull/76

sophiewigmore commented 6 months ago

Alright! It's been merged, so should we update the branch and re-run?

pacostas commented 6 months ago

@sophiewigmore There is an issue https://github.com/paketo-community/ubi-nodejs-extension/actions/runs/7936669801/job/21783871482 while pushing the images to GCR as a result the next step which is to push it on dockerhub has been canceled so the latest change is not in dockerhub as a result the tests will fail as it will fetch the previous version of ubi-nodejs-extension 0.0.2 instead of 0.0.3 This issue is on any github action that tries to push on docker hub or on GCR e.x. here https://github.com/paketo-community/ubi-base-stack/actions/runs/7975863574/job/21775051704 . I think it has to do something with permissions

Requesting bear token: invalid status code from registry 403 (Forbidden)"

Is there a way to validate that the user on dockerhub and on GCR has proper permissions for pushing images to their registries?

Also before merging this PR, I would suggest merging this one first https://github.com/paketo-community/ubi-base-stack/pull/41 no matter if we cant push to GCR or dockerhub ATM. The reason is that the current PR alters the strucure of the tests and is preferable to rebase refactoring at the same time the changes from the PR https://github.com/paketo-community/ubi-base-stack/pull/41 that adds the stack instead of the opposite.

sophiewigmore commented 6 months ago

You guys should feel free to merge #41 when you want to, I gave a thumbs up!

mhdawson commented 6 months ago

I believe that the issue referenced by @pacostas in https://github.com/paketo-community/ubi-base-stack/pull/35#issuecomment-1956350697 is related to - https://github.com/orgs/paketo-buildpacks/discussions/253, following up with Daniel to figure out what we should do to resolve.

pacostas commented 6 months ago

You guys should feel free to merge #41 when you want to, I gave a thumbs up!

Thank you :) ! We are waiting to resolve the issue with the GCR

paketo-community / ubi-base-stack