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

Tolerate changelist.format=%d.v%s #14

Merged
merged 2 commits into from
Mar 2, 2021
Merged

Tolerate changelist.format=%d.v%s #14

merged 2 commits into from
Mar 2, 2021

Conversation

jglick
Copy link
Collaborator

@jglick jglick commented Mar 2, 2021

Noticed in jenkinsci/jenkins-test-harness#277 that incremental deployment was not working, due to jenkinsci/incrementals-tools#19 breaking the assumption about the final version format. Amends #10 which was introduced for the previously recommended JEP-229 format.

@halkeye
Copy link
Member

halkeye commented Mar 2, 2021

I feel like this would break tests, but I also don't see CI results being run.
looks like even if it was run, tests are not run inside the docker image

so /shrug

@jglick
Copy link
Collaborator Author

jglick commented Mar 2, 2021

Indeed CI seems to be off. Running make locally…

@jglick
Copy link
Collaborator Author

jglick commented Mar 2, 2021

Oh I see what you mean, that does not run tests. In abcdf72 you did not update README.md it seems: still says to make check.

I tried to

diff --git a/Dockerfile b/Dockerfile
index ef074ec..fab5076 100644
--- a/Dockerfile
+++ b/Dockerfile
@@ -17,4 +17,6 @@ RUN npm install
 # Bundle app source
 COPY . .
 
+RUN npm test
+
 CMD [ "node", "index.js" ]

but this complains something about Mocha.

@halkeye
Copy link
Member

halkeye commented Mar 2, 2021

yea the docker file only runs npm install with NODE_ENV=production

I would say don't worry about it, if i get a min I'll make a multi stage docker image that runs tests first

@jglick
Copy link
Collaborator Author

jglick commented Mar 2, 2021

Anyway, do you own this service? I apparently have write permissions to the repo but I should not be merging PRs.

@timja timja merged commit 68e2355 into master Mar 2, 2021
@timja timja deleted the changelist.format branch March 2, 2021 19:51
@timja
Copy link
Member

timja commented Mar 2, 2021

i assume this needs a version bump and a tag for release @halkeye ?

@timja timja added the enhancement New feature or request label Mar 2, 2021
@halkeye
Copy link
Member

halkeye commented Mar 2, 2021

yea, its the standard docker ci system, so bump version and generate tag and ci will take care of the rest

@jglick
Copy link
Collaborator Author

jglick commented Mar 3, 2021

@timja
Copy link
Member

timja commented Mar 3, 2021

updateCli was disabled for some reason, seems fixed now, PR is up at jenkins-infra/kubernetes-management#923

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants