Skip to content
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

Stop overriding intree extension versions on build #6239

Merged
merged 2 commits into from Oct 3, 2022

Conversation

Nokel81
Copy link
Collaborator

@Nokel81 Nokel81 commented Sep 13, 2022

Signed-off-by: Sebastian Malton sebastian@malton.name

Signed-off-by: Sebastian Malton <sebastian@malton.name>
Signed-off-by: Sebastian Malton <sebastian@malton.name>
@Nokel81 Nokel81 added chore area/extension Something to related to the extension api labels Sep 13, 2022
@Nokel81 Nokel81 added this to the 6.1.0 milestone Sep 13, 2022
@Nokel81 Nokel81 requested a review from a team as a code owner September 13, 2022 18:29
@Nokel81 Nokel81 requested review from ixrock and jim-docker and removed request for a team September 13, 2022 18:29
Comment on lines -66 to -68
.PHONY: update-extension-locks
update-extension-locks:
$(foreach dir, $(extensions), (cd $(dir) && rm package-lock.json && ../../node_modules/.bin/npm install --package-lock-only);)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unused as we no longer keep those package-lock files in git

@@ -92,7 +87,7 @@ src/extensions/npm/extensions/dist: src/extensions/npm/extensions/node_modules
yarn compile:extension-types

src/extensions/npm/extensions/node_modules: src/extensions/npm/extensions/package.json
cd src/extensions/npm/extensions/ && ../../../../node_modules/.bin/npm install --no-audit --no-fund
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We aren't really using the package-lock file anyway since we override it whenever we do a make build-extension-types invocation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wondering should we include package-locks... anyway that is a story for an another PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah probably. Once these are libraries and build themselves for the releases.

@@ -1,6 +1,6 @@
{
"name": "kube-object-event-status",
"version": "0.0.1",
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This version needs to be higher than any version currently in the wild but we also don't need to set it on every build. Only when we change the extension.

Comment on lines +19 to +20
"@material-ui/core": "^4.12.3",
"@types/node": "^16.11.58",
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are changed to make them the same as what we have in the root package.json

@Nokel81 Nokel81 modified the milestones: 6.1.0, 6.2.0 Sep 14, 2022
@Nokel81
Copy link
Collaborator Author

Nokel81 commented Sep 14, 2022

Marking this as 6.2.0 so that it has more time to bake. Also do not merge until after we cut 6.1.0

@@ -1,6 +1,6 @@
{
"name": "lens-metrics-cluster-feature",
"version": "0.0.1",
"version": "6.1.0",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you tell more about reason to set extension version numbers same as Lens version itself? Also, by your design, should we change those versions on every app release? For example, next release would be 6.2.0 and extensions will stay with 6.1.0?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason that was to make sure that any new updates to the extensions ensure that they are always up to date (because newer versions will be overridden during update). But if there are no changes then I don't see the point of ensuring that.

If we make a change to the extensions then we should bump the versions.

@Nokel81
Copy link
Collaborator Author

Nokel81 commented Sep 16, 2022

@jakolehm PTAL

@@ -92,7 +87,7 @@ src/extensions/npm/extensions/dist: src/extensions/npm/extensions/node_modules
yarn compile:extension-types

src/extensions/npm/extensions/node_modules: src/extensions/npm/extensions/package.json
cd src/extensions/npm/extensions/ && ../../../../node_modules/.bin/npm install --no-audit --no-fund
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wondering should we include package-locks... anyway that is a story for an another PR.

@Nokel81 Nokel81 merged commit e6396b7 into master Oct 3, 2022
@Nokel81 Nokel81 deleted the remove-bundled-extension-version-overriding branch October 3, 2022 14:36
@Nokel81 Nokel81 mentioned this pull request Nov 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/extension Something to related to the extension api chore
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants