-
Notifications
You must be signed in to change notification settings - Fork 11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat!: Removing unused ACTOR_BASE_DOCKER_IMAGES and ACTOR_BASE_DOCKER_IMAGE_DEFAULT constants #376
Conversation
This constant is not currently used anywhere, and the repo with docker images should serve as a single source of truth.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, but we probably can remove the other const too.
packages/consts/src/consts.ts
Outdated
* Default image from ACTOR_BASE_DOCKER_IMAGES. | ||
*/ | ||
export const ACTOR_BASE_DOCKER_IMAGE_DEFAULT = ACTOR_BASE_DOCKER_IMAGES[0].name; | ||
export const ACTOR_BASE_DOCKER_IMAGE_DEFAULT = 'apify/actor-node'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can remove this const too? We have no notion of "default base Docker image" anymore, and it's only used in two files in integration tests, where we can/should hardcode it anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, right, I see it's only used on tests and there it can be hardcoded for sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, fixed in https://github.com/apify/apify-core/pull/10579
This constant is not currently used anywhere, and the repo with docker images should serve as a single source of truth.