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

feat(contributing): add SFTP configuration #8

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

jnijdam
Copy link

@jnijdam jnijdam commented Mar 30, 2022

PR progress checklist (to be filled in by reviewers)

  • Changes to documentation are appropriate (or tick if not required)
  • Changes to tests are appropriate (or tick if not required)
  • Reviews completed

What type of PR is this?

Primary type

  • [feat] A new feature

Secondary type

  • [docs] Documentation changes
  • [test] Adding missing or correcting existing tests

Does this PR introduce a BREAKING CHANGE?

No.

Related issues and/or pull requests

Describe the changes you're proposing

Pillar / config required to test the proposed changes

Debug log showing how the proposed changes work

Documentation checklist

  • Updated the README (e.g. Available states).

Testing checklist

  • Included in Kitchen (i.e. under state_top).
  • Covered by new/existing tests (e.g. InSpec, Serverspec, etc.).

Additional context

@jnijdam jnijdam force-pushed the feature/jn/add-sftp branch 5 times, most recently from fc63c80 to a38031d Compare March 31, 2022 08:33
@jnijdam jnijdam requested a review from a team as a code owner March 31, 2022 08:33
@jnijdam jnijdam force-pushed the feature/jn/add-sftp branch 6 times, most recently from 4f610b3 to d9407e4 Compare April 6, 2022 07:58
Copy link
Member

@daks daks left a comment

Choose a reason for hiding this comment

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

I made several comments to the code.

As noted, for now this code makes a breaking change because sftp module is activated which it was not previously.

Another problem is, when activating the sftp state (in kitchen.yml) converge do not pass. I think kitchen/inspec tests should be added to be confident in how this code works. See kitchen.yml suite section to add one to the default one.

test/salt/pillar/default.sls Outdated Show resolved Hide resolved
proftpd/map.jinja Outdated Show resolved Hide resolved
proftpd/sftp.yaml Outdated Show resolved Hide resolved
@jnijdam jnijdam force-pushed the feature/jn/add-sftp branch 16 times, most recently from 2da6cf2 to 55abc2c Compare April 14, 2022 13:36
@jnijdam jnijdam force-pushed the feature/jn/add-sftp branch 14 times, most recently from b5bceba to a8dac01 Compare May 12, 2022 15:04
@jnijdam jnijdam force-pushed the feature/jn/add-sftp branch 6 times, most recently from 2ff6369 to cdf905f Compare May 24, 2022 15:33
@daks daks force-pushed the feature/jn/add-sftp branch 2 times, most recently from 2211721 to 777d0c8 Compare July 12, 2022 09:32
@daks
Copy link
Member

daks commented Jul 13, 2022

@jnijdam I'm working on improving your PR which is not functional right now. I'll add commits to it once I get a working setup

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

Successfully merging this pull request may close these issues.

2 participants