Skip to content

Fix breaking change regression of PR #429#434

Merged
rwaffen merged 20 commits intovoxpupuli:masterfrom
diLLec:master
Mar 27, 2025
Merged

Fix breaking change regression of PR #429#434
rwaffen merged 20 commits intovoxpupuli:masterfrom
diLLec:master

Conversation

@diLLec
Copy link
Copy Markdown
Contributor

@diLLec diLLec commented Feb 25, 2025

Pull Request (PR) description

Optimization for PR Add "change database password" parameter - JIRA >10.3

This Pull Request (PR) fixes the following issues

  • make change_dbpassword only be effective after version 10.3.0
  • add tests

Adding version constraint for the effectiveness of the change_dbpassword parameter to greater than 10.3.0. In all versions prior to 10.3.0 the password is written in plain text.
@diLLec
Copy link
Copy Markdown
Contributor Author

diLLec commented Feb 25, 2025

I really tried to add an acceptance test for JIRA 10.3.3 but I think we would need to limit those to certain modern OS versions like CentOS 9. Otherwise the prerequisites on MySQL and JAVA Version would not be able to be met. Unfortunately, I don't know how to limit the acceptance tests to a certain OS. If someone knows, please contact me.

Comment thread manifests/init.pp Outdated
Comment thread templates/dbconfig.xml.epp Outdated
Comment thread manifests/config.pp Outdated
Comment thread manifests/init.pp Outdated
@SimonHoenscheid
Copy link
Copy Markdown
Member

@diLLec looks good to me. I asked for two other reviewers

@SimonHoenscheid
Copy link
Copy Markdown
Member

@diLLec can you sqash your commits so there is just one? I think most commits can be fixups.

git rebase -i HEAD~13 should initiate the rebase

Comment thread .vscode/settings.json Outdated
@bastelfreak
Copy link
Copy Markdown
Member

@diLLec thanks for the PR. As you mentioned, I think an acceptance test makes sense. https://github.com/voxpupuli/puppet-nodejs/blob/master/spec/acceptance/class_spec.rb#L38 has an example of limiting a test to specific operating systems. Does that help you?

@SimonHoenscheid SimonHoenscheid added the bug Something isn't working label Feb 27, 2025
Copy link
Copy Markdown
Member

@kenyon kenyon left a comment

Choose a reason for hiding this comment

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

We can squash commits on merge.

Comment thread spec/classes/jira_config_spec.rb Outdated
Comment thread spec/classes/jira_config_spec.rb Outdated
@h-haaks
Copy link
Copy Markdown
Contributor

h-haaks commented Feb 28, 2025

The title of this pull request ends up in the change log.
Could you change the title to better address the fact that this fixes a regression introduced in #429 ?

@h-haaks
Copy link
Copy Markdown
Contributor

h-haaks commented Feb 28, 2025

There is another regression as well.

when provisioning jira 10.3.x on a fresh server without specifying change_dbpassword = true, the password ends up being set to {ATL_SECURED} before the first server startup.

Solving this regression requires implementing some facts to figure out if jira is already installed or not.
It's probably best to address this in another issue/pull request.

@diLLec
Copy link
Copy Markdown
Contributor Author

diLLec commented Feb 28, 2025

After a day of testing and learning I finally managed to bring the acceptance test and the facts to work. Please review again. Locally the acceptance tests are building fine with debian11-64 and almalinux8-64 and almalinux9-64

@diLLec diLLec changed the title Optimize "change database password" parameter - JIRA >10.3 #429 Fix breaking change regression of PR #429 Mar 3, 2025
@SimonHoenscheid
Copy link
Copy Markdown
Member

Can we merge this Change?

Copy link
Copy Markdown

@vchepkov vchepkov left a comment

Choose a reason for hiding this comment

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

I would pass a path to the config file instead of relying on user's home.
manifest uses jira::homedir, imho, that should be used

Comment thread lib/puppet/functions/jira/is_installed.rb
Comment thread spec/acceptance/default_parameters_jira_10_spec.rb Outdated
Comment thread spec/acceptance/default_parameters_jira_10_spec.rb Outdated
EOS

pp = <<-EOS
# The output of `systemctl status postgresql` is non ascii which
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I have some questions here. Why does that even break the exec resource? And is that a patch that should be added to the postgresql module instead?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This I've copied from the existing default_parameters_spec.rb. I don't know if it is still needed.

Comment thread spec/acceptance/default_parameters_jira_10_spec.rb
@diLLec diLLec force-pushed the master branch 3 times, most recently from 7765047 to b8e4777 Compare March 26, 2025 07:29
@diLLec
Copy link
Copy Markdown
Contributor Author

diLLec commented Mar 26, 2025

I would pass a path to the config file instead of relying on user's home. manifest uses jira::homedir, imho, that should be used

OK - added that

@diLLec
Copy link
Copy Markdown
Contributor Author

diLLec commented Mar 26, 2025

From my point of view, this MR can be merged: all tests are OK and all the requested changes are included.

@rwaffen
Copy link
Copy Markdown
Member

rwaffen commented Mar 27, 2025

okay, nice, just was looking for this change 😃 will merge and release

@rwaffen rwaffen merged commit c1d27bc into voxpupuli:master Mar 27, 2025
25 checks passed
cegeka-jenkins pushed a commit to cegeka/puppet-jira that referenced this pull request Sep 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working needs-squash

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants