saltstack-formulas / php-formula

http://docs.saltstack.com/en/latest/topics/development/conventions/formulas.html
Other
57 stars 232 forks source link

add php version to redis for debian in map.jinja #229

Closed Ahummeling closed 3 years ago

Ahummeling commented 3 years ago

PR progress checklist (to be filled in by reviewers)


What type of PR is this?

Primary type

Secondary type

Does this PR introduce a BREAKING CHANGE?

No.

Related issues and/or pull requests

Describe the changes you're proposing

Set the php version in the map for debian module redis, make it install php8.0-redis since php-redis results in 7.3 on server and 8.0 on another server. I am not aware for the initial reason for not including the php version.

Pillar / config required to test the proposed changes

Debug log showing how the proposed changes work

----------
          ID: php_install_redis
    Function: pkg.installed
        Name: redis
      Result: True
     Comment: The following packages were installed/updated: php8.0-redis
     Started: 16:11:02.328496
    Duration: 3485.581 ms
     Changes:   
              ----------
              php-redis:
                  ----------
                  new:
                      5.3.4+4.3.0-1+0~20210329.35+debian10~1.gbpdc7ead
                  old:
                      5.3.1+4.3.0-1+0~20200708.25+debian10~1.gbp800f71
              php8.0-redis:
                  ----------
                  new:
                      5.3.4+4.3.0-1+0~20210329.35+debian10~1.gbpdc7ead
                  old:

Documentation checklist

Testing checklist

Additional context

I'm sorry if this PR seems half assed. I'm a total noob when it comes to salt, feel free to close the PR if it doesn't meet the standards.

johnkeates commented 3 years ago

It seems the commitlint is not happy with the way the commit is named; if you check https://github.com/saltstack-formulas/.github/blob/master/CONTRIBUTING.rst you'll find the commit message formatting notes and that should clear this up.

I have started the CI anyway to validate the code change as-is, but before this can be merged the commit message should probably be updated.

Ahummeling commented 3 years ago

Right, I've squashed my commits and wrote a clean commit message. Also updated the testcases that broke as a result of the change.

myii commented 3 years ago

Apologies for the delay, @Ahummeling. The failing tests should be simple enough to resolve. Just want to confirm that @sticky-note is happy with the implementation.

Ahummeling commented 3 years ago

Apologies for the seemingly strange commits / pushes all over. I'm having a ton of difficulty figuring out the github webui. I'm used to using gitlab. I don't think I should've pressed that fetch upstream button, it did not do what I thought it did. The changelist is now showing a ton of changes that come from squashing the commits incorrectly. I'll see if I can resolve those, so a review can be done correctly.

Ahummeling commented 3 years ago

Seems like the tests are passing again, at least the ones that could be triggered without approval. If anything needs to happen still, I will pick it up asap

myii commented 3 years ago

Apologies for the seemingly strange commits / pushes all over. I'm having a ton of difficulty figuring out the github webui. I'm used to using gitlab. I don't think I should've pressed that fetch upstream button, it did not do what I thought it did. The changelist is now showing a ton of changes that come from squashing the commits incorrectly. I'll see if I can resolve those, so a review can be done correctly.

@Ahummeling Not to worry, we can always rebase this PR to make things cleaner. If you're comfortable doing it at your end, you can rebase your commits and then force push back to this PR's branch. If not, we're able to do that from the GitHub interface at the time of merging.

johnkeates commented 3 years ago

I gave the extra checks a little push so we can see if it works everywhere 👍

myii commented 3 years ago

Merged, thanks for the PR @Ahummeling -- and once again, apologies for the delay. Appreciate the help along the way, @johnkeates.

saltstack-formulas-travis commented 3 years ago

:tada: This PR is included in version 1.5.1 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket: