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

Jump to bottom

Properly and consistently report changes in 'files' promises #4196

Merged
merged 28 commits into from Jun 10, 2020
Merged

Properly and consistently report changes in 'files' promises #4196

merged 28 commits into from Jun 10, 2020

Conversation

vpodzime
Copy link
Contributor

@vpodzime vpodzime commented May 26, 2020 •
edited

@vpodzime vpodzime requested a review from olehermanse May 26, 2020 12:12
@vpodzime vpodzime force-pushed the master-report_promise_changes branch from 979fd3f to 2390479 Compare May 26, 2020 12:39
@cfengine cfengine deleted a comment from lgtm-com bot May 26, 2020
@cfengine cfengine deleted a comment from lgtm-com bot May 27, 2020
nickanderson
nickanderson previously approved these changes May 27, 2020
View reviewed changes
Copy link
Member

@nickanderson nickanderson left a comment

Choose a reason for hiding this comment

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

Overall it looks good to me. Only one place I might consider additional changes to try and make the test result easier to expect.

tests/acceptance/10_files/09_insert_lines/nothing_to_repeat_error.cf Show resolved Hide resolved
cf-agent/files_changes.c Show resolved Hide resolved
tests/acceptance/10_files/02_maintain/changes_update_hashes.cf Show resolved Hide resolved
cf-agent/verify_files_utils.c Show resolved Hide resolved
@nickanderson nickanderson mentioned this pull request Jun 1, 2020
Closed
olehermanse
olehermanse reviewed Jun 2, 2020
View reviewed changes
Copy link
Member

@olehermanse olehermanse left a comment

Choose a reason for hiding this comment

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

Here is a partial review. I will get back to the last 9 commits later.

libpromises/eval_context.c Outdated Show resolved Hide resolved
libpromises/eval_context.h Show resolved Hide resolved
libpromises/files_lib.c Outdated Show resolved Hide resolved
cf-agent/verify_files_utils.c Outdated Show resolved Hide resolved
cf-agent/verify_files_utils.c Outdated Show resolved Hide resolved
cf-agent/files_changes.c Outdated Show resolved Hide resolved
cf-agent/acl_posix.c Outdated Show resolved Hide resolved
cf-agent/verify_files_utils.c Show resolved Hide resolved
cf-agent/verify_files_utils.c Show resolved Hide resolved
cf-agent/verify_files_utils.c Outdated Show resolved Hide resolved
nickanderson
nickanderson reviewed Jun 2, 2020
View reviewed changes
Copy link
Member

@nickanderson nickanderson left a comment

Choose a reason for hiding this comment

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

Nice one!

@cfengine cfengine deleted a comment from lgtm-com bot Jun 2, 2020
@lgtm-com
Copy link

lgtm-com bot commented Jun 3, 2020

This pull request introduces 101 alerts and fixes 9 when merging 0ca76d2 into 2da72cc - view on LGTM.com

new alerts:

  • 95 for Pointer argument is dereferenced without checking for NULL
  • 6 for Commented-out code

fixed alerts:

  • 7 for Pointer argument is dereferenced without checking for NULL
  • 1 for Futile conditional
  • 1 for Time-of-check time-of-use filesystem race condition

@cfengine cfengine deleted a comment from lgtm-com bot Jun 3, 2020
olehermanse
olehermanse requested changes Jun 3, 2020
View reviewed changes
Copy link
Member

@olehermanse olehermanse left a comment •
edited

Choose a reason for hiding this comment

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

Looks good overall. Hard to thoroughly review everything, but the only major issue I saw was the missing continue. Fix that, read through my comments and request another review :)

cf-agent/files_editxml.c Show resolved Hide resolved
cf-agent/verify_files.c Outdated Show resolved Hide resolved
@vpodzime
Copy link
Contributor Author

vpodzime commented Jun 4, 2020 •
edited

Pushed a commit fixing the manage_variable_values_ini.cf test, but the output from it now looks like this:

----------------------------------------------------------------------
./lib/files/manage_variable_values_ini.cf 
----------------------------------------------------------------------
2020-06-04T13:17:37+0200    error: UNTRUSTED: Module directory /home/vpodzime/sources/cfe/masterfiles/tests/acceptance/workdir/__lib_files_manage_variable_values_ini_cf/modules (mode 775) was not private!
   error: UNTRUSTED: Module directory /home/vpodzime/sources/cfe/masterfiles/tests/acceptance/workdir/__lib_files_manage_variable_values_ini_cf/modules (mode 775) was not private!
R: test description: Test expected behavior of manage_variable_values_ini
2020-06-04T13:17:38+0200    error: No matched line to edit fields of for pattern '#+\s*keyone\s*=.*' in '/home/vpodzime/sources/cfe/masterfiles/tests/acceptance/workdir/__lib_files_manage_variable_values_ini_cf/tmp/TEST.cfengine.actual'
2020-06-04T13:17:38+0200    error: Errors encountered when actuating fields_edit promise '#+\s*keyone\s*=.*'
2020-06-04T13:17:38+0200    error: No matched line to edit fields of for pattern '#+\s*keytwo\s*=.*' in '/home/vpodzime/sources/cfe/masterfiles/tests/acceptance/workdir/__lib_files_manage_variable_values_ini_cf/tmp/TEST.cfengine.actual'
2020-06-04T13:17:38+0200    error: Errors encountered when actuating fields_edit promise '#+\s*keytwo\s*=.*'
2020-06-04T13:17:38+0200    error: No matched line to edit fields of for pattern '\s*keyone\s*=.*' in '/home/vpodzime/sources/cfe/masterfiles/tests/acceptance/workdir/__lib_files_manage_variable_values_ini_cf/tmp/TEST.cfengine.actual'
2020-06-04T13:17:38+0200    error: Errors encountered when actuating fields_edit promise '\s*keyone\s*=.*'
2020-06-04T13:17:38+0200    error: No matched line to edit fields of for pattern '\s*keytwo\s*=.*' in '/home/vpodzime/sources/cfe/masterfiles/tests/acceptance/workdir/__lib_files_manage_variable_values_ini_cf/tmp/TEST.cfengine.actual'
2020-06-04T13:17:38+0200    error: Errors encountered when actuating fields_edit promise '\s*keytwo\s*=.*'
2020-06-04T13:17:38+0200    error: No matched line to edit fields of for pattern '#+\s*variablename4\s*=.*' in '/home/vpodzime/sources/cfe/masterfiles/tests/acceptance/workdir/__lib_files_manage_variable_values_ini_cf/tmp/TEST.cfengine.actual'
2020-06-04T13:17:38+0200    error: Errors encountered when actuating fields_edit promise '#+\s*variablename4\s*=.*'
2020-06-04T13:17:38+0200    error: No matched line to edit fields of for pattern '#+\s*variablename\s*=.*' in '/home/vpodzime/sources/cfe/masterfiles/tests/acceptance/workdir/__lib_files_manage_variable_values_ini_cf/tmp/TEST.cfengine.actual'
2020-06-04T13:17:38+0200    error: Errors encountered when actuating fields_edit promise '#+\s*variablename\s*=.*'
2020-06-04T13:17:38+0200    error: No matched line to edit fields of for pattern '#+\s*variablename5\s*=.*' in '/home/vpodzime/sources/cfe/masterfiles/tests/acceptance/workdir/__lib_files_manage_variable_values_ini_cf/tmp/TEST.cfengine.actual'
2020-06-04T13:17:38+0200    error: Errors encountered when actuating fields_edit promise '#+\s*variablename5\s*=.*'
2020-06-04T13:17:38+0200    error: No matched line to edit fields of for pattern '#+\s*variablename2\s*=.*' in '/home/vpodzime/sources/cfe/masterfiles/tests/acceptance/workdir/__lib_files_manage_variable_values_ini_cf/tmp/TEST.cfengine.actual'
2020-06-04T13:17:38+0200    error: Errors encountered when actuating fields_edit promise '#+\s*variablename2\s*=.*'
2020-06-04T13:17:38+0200    error: No matched line to edit fields of for pattern '#+\s*variablename3\s*=.*' in '/home/vpodzime/sources/cfe/masterfiles/tests/acceptance/workdir/__lib_files_manage_variable_values_ini_cf/tmp/TEST.cfengine.actual'
2020-06-04T13:17:38+0200    error: Errors encountered when actuating fields_edit promise '#+\s*variablename3\s*=.*'
2020-06-04T13:17:38+0200    error: No matched line to edit fields of for pattern '#+\s*variablename8\s*=.*' in '/home/vpodzime/sources/cfe/masterfiles/tests/acceptance/workdir/__lib_files_manage_variable_values_ini_cf/tmp/TEST.cfengine.actual'
2020-06-04T13:17:38+0200    error: Errors encountered when actuating fields_edit promise '#+\s*variablename8\s*=.*'
2020-06-04T13:17:38+0200    error: No matched line to edit fields of for pattern '#+\s*variablename6\s*=.*' in '/home/vpodzime/sources/cfe/masterfiles/tests/acceptance/workdir/__lib_files_manage_variable_values_ini_cf/tmp/TEST.cfengine.actual'
2020-06-04T13:17:38+0200    error: Errors encountered when actuating fields_edit promise '#+\s*variablename6\s*=.*'
2020-06-04T13:17:38+0200    error: No matched line to edit fields of for pattern '#+\s*variablename7\s*=.*' in '/home/vpodzime/sources/cfe/masterfiles/tests/acceptance/workdir/__lib_files_manage_variable_values_ini_cf/tmp/TEST.cfengine.actual'
2020-06-04T13:17:38+0200    error: Errors encountered when actuating fields_edit promise '#+\s*variablename7\s*=.*'
2020-06-04T13:17:38+0200    error: No matched line to edit fields of for pattern '\s*variablename4\s*=.*' in '/home/vpodzime/sources/cfe/masterfiles/tests/acceptance/workdir/__lib_files_manage_variable_values_ini_cf/tmp/TEST.cfengine.actual'
2020-06-04T13:17:38+0200    error: Errors encountered when actuating fields_edit promise '\s*variablename4\s*=.*'
2020-06-04T13:17:38+0200    error: No matched line to edit fields of for pattern '\s*variablename\s*=.*' in '/home/vpodzime/sources/cfe/masterfiles/tests/acceptance/workdir/__lib_files_manage_variable_values_ini_cf/tmp/TEST.cfengine.actual'
2020-06-04T13:17:38+0200    error: Errors encountered when actuating fields_edit promise '\s*variablename\s*=.*'
2020-06-04T13:17:38+0200    error: No matched line to edit fields of for pattern '\s*variablename5\s*=.*' in '/home/vpodzime/sources/cfe/masterfiles/tests/acceptance/workdir/__lib_files_manage_variable_values_ini_cf/tmp/TEST.cfengine.actual'
2020-06-04T13:17:38+0200    error: Errors encountered when actuating fields_edit promise '\s*variablename5\s*=.*'
2020-06-04T13:17:38+0200    error: No matched line to edit fields of for pattern '\s*variablename2\s*=.*' in '/home/vpodzime/sources/cfe/masterfiles/tests/acceptance/workdir/__lib_files_manage_variable_values_ini_cf/tmp/TEST.cfengine.actual'
2020-06-04T13:17:38+0200    error: Errors encountered when actuating fields_edit promise '\s*variablename2\s*=.*'
2020-06-04T13:17:38+0200    error: No matched line to edit fields of for pattern '\s*variablename3\s*=.*' in '/home/vpodzime/sources/cfe/masterfiles/tests/acceptance/workdir/__lib_files_manage_variable_values_ini_cf/tmp/TEST.cfengine.actual'
2020-06-04T13:17:38+0200    error: Errors encountered when actuating fields_edit promise '\s*variablename3\s*=.*'
2020-06-04T13:17:38+0200    error: No matched line to edit fields of for pattern '\s*variablename8\s*=.*' in '/home/vpodzime/sources/cfe/masterfiles/tests/acceptance/workdir/__lib_files_manage_variable_values_ini_cf/tmp/TEST.cfengine.actual'
2020-06-04T13:17:38+0200    error: Errors encountered when actuating fields_edit promise '\s*variablename8\s*=.*'
2020-06-04T13:17:38+0200    error: No matched line to edit fields of for pattern '\s*variablename6\s*=.*' in '/home/vpodzime/sources/cfe/masterfiles/tests/acceptance/workdir/__lib_files_manage_variable_values_ini_cf/tmp/TEST.cfengine.actual'
2020-06-04T13:17:38+0200    error: Errors encountered when actuating fields_edit promise '\s*variablename6\s*=.*'
2020-06-04T13:17:38+0200    error: No matched line to edit fields of for pattern '\s*variablename7\s*=.*' in '/home/vpodzime/sources/cfe/masterfiles/tests/acceptance/workdir/__lib_files_manage_variable_values_ini_cf/tmp/TEST.cfengine.actual'
2020-06-04T13:17:38+0200    error: Errors encountered when actuating fields_edit promise '\s*variablename7\s*=.*'
2020-06-04T13:17:38+0200    error: No matched line to edit fields of for pattern '#+\s*keyone\s*=.*' in '/home/vpodzime/sources/cfe/masterfiles/tests/acceptance/workdir/__lib_files_manage_variable_values_ini_cf/tmp/TEST.cfengine.actual'
2020-06-04T13:17:38+0200    error: Errors encountered when actuating fields_edit promise '#+\s*keyone\s*=.*'
2020-06-04T13:17:38+0200    error: No matched line to edit fields of for pattern '#+\s*keytwo\s*=.*' in '/home/vpodzime/sources/cfe/masterfiles/tests/acceptance/workdir/__lib_files_manage_variable_values_ini_cf/tmp/TEST.cfengine.actual'
2020-06-04T13:17:38+0200    error: Errors encountered when actuating fields_edit promise '#+\s*keytwo\s*=.*'
2020-06-04T13:17:38+0200    error: No matched line to edit fields of for pattern '\s*keyone\s*=.*' in '/home/vpodzime/sources/cfe/masterfiles/tests/acceptance/workdir/__lib_files_manage_variable_values_ini_cf/tmp/TEST.cfengine.actual'
2020-06-04T13:17:38+0200    error: Errors encountered when actuating fields_edit promise '\s*keyone\s*=.*'
2020-06-04T13:17:38+0200    error: No matched line to edit fields of for pattern '\s*keytwo\s*=.*' in '/home/vpodzime/sources/cfe/masterfiles/tests/acceptance/workdir/__lib_files_manage_variable_values_ini_cf/tmp/TEST.cfengine.actual'
2020-06-04T13:17:38+0200    error: Errors encountered when actuating fields_edit promise '\s*keytwo\s*=.*'
R: Diff command: /usr/bin/diff -u /home/vpodzime/sources/cfe/masterfiles/tests/acceptance/workdir/__lib_files_manage_variable_values_ini_cf/tmp/TEST.cfengine.expected /home/vpodzime/sources/cfe/masterfiles/tests/acceptance/workdir/__lib_files_manage_variable_values_ini_cf/tmp/TEST.cfengine.actual 2>/dev/null
R: /home/vpodzime/sources/cfe/masterfiles/tests/acceptance/./lib/files/manage_variable_values_ini.cf Pass
R: dcs_pass: Explicitly passing...

Return code is 0.

  ==> Pass

@lgtm-com
Copy link

lgtm-com bot commented Jun 4, 2020

This pull request introduces 103 alerts and fixes 9 when merging de507ad into 2da72cc - view on LGTM.com

new alerts:

  • 97 for Pointer argument is dereferenced without checking for NULL
  • 6 for Commented-out code

fixed alerts:

  • 7 for Pointer argument is dereferenced without checking for NULL
  • 1 for Futile conditional
  • 1 for Time-of-check time-of-use filesystem race condition

@vpodzime
Copy link
Contributor Author

vpodzime commented Jun 4, 2020

@cf-bottom jenkins with exotics, please

@cf-bottom
Copy link

Sure, I triggered a build:

Build Status

(with exotics)

Jenkins: https://ci.cfengine.com/job/pr-pipeline/5214/

Packages: http://buildcache.cfengine.com/packages/testing-pr/jenkins-pr-pipeline-5214/

@vpodzime vpodzime force-pushed the master-report_promise_changes branch 2 times, most recently from 763595e to f2715a3 Compare June 9, 2020 09:36
@cfengine cfengine deleted a comment from lgtm-com bot Jun 9, 2020
@vpodzime
Copy link
Contributor Author

vpodzime commented Jun 9, 2020

@cf-bottom one last jenkins with exotics, please! 🤞

@vpodzime vpodzime requested a review from olehermanse June 9, 2020 09:37
@cf-bottom
Copy link

Alright, I triggered a build:

Build Status

(with exotics)

Jenkins: https://ci.cfengine.com/job/pr-pipeline/5250/

Packages: http://buildcache.cfengine.com/packages/testing-pr/jenkins-pr-pipeline-5250/

@lgtm-com
Copy link

lgtm-com bot commented Jun 9, 2020

This pull request introduces 98 alerts and fixes 9 when merging f2715a3 into 806cbaa - view on LGTM.com

new alerts:

  • 97 for Pointer argument is dereferenced without checking for NULL
  • 1 for FIXME comment

fixed alerts:

  • 7 for Pointer argument is dereferenced without checking for NULL
  • 1 for Futile conditional
  • 1 for Time-of-check time-of-use filesystem race condition

olehermanse
olehermanse previously approved these changes Jun 9, 2020
View reviewed changes
Copy link
Member

@olehermanse olehermanse left a comment

Choose a reason for hiding this comment

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

LGTM, please review the jenkins failure before merging.

@vpodzime
Copy link
Contributor Author

LGTM, please review the jenkins failure before merging.

Rebuild passed on Solaris 11: Build Status

@vpodzime
Add a functions for recording changes, failures, etc.
a402fff 
More high-level function than just 'Log()' which, unlike
'cfPS()', say nothing about the state of the whole promise
because one promise can cause multiple changes on the system.

Ticket: ENT-5291
Changelog: None
@vpodzime
Make it possible to check if MakeParentDirectory() created something
e7a2989 
So that we can report the fact.

Ticket: ENT-5291
Changelog: None
@vpodzime
Properly report changes in the CfCreateFile function
2a5d1dd 
It should report all changes it makes. And it should use
LogChange() instead of cfPS() for doing so because cfPS() is for
reporting status of a whole promise.

Ticket: ENT-5291
Changelog: Directory and file creations are now properly reported as 'info' messages
@vpodzime
Record that no change was required for file (not) existing as promised
332e37b 
Ticket: ENT-5291
Changelog: None
@vpodzime
Properly report changes for files.transformer
d92fe6a 
Ticket: ENT-5291
Changelog: None
@vpodzime
Properly report changes in VerifyName()
3b7d216 
Ticket: ENT-5291
Changelog: None
@vpodzime
Properly report changes in VerifyDelete
b2ae02e 
Ticket: ENT-5291
Changelog: None
@vpodzime
Properly report about changes in TouchFile()
b89cac8 
Ticket: ENT-5291
Changelog: None
@vpodzime
Properly report changes in VerifyFileAttributes()
c62cd9d 
Ticket: ENT-5291
Changelog: None
@vpodzime
Properly report changes in File Integrity Monitoring
3dc1e55 
Ticket: ENT-5291
Changelog: None
@vpodzime
Properly report ACL changes
039eb8a 
Ticket: ENT-5291
Changelog: None
@vpodzime
Properly report about changes in DepthSearch()
b391032 
Ticket: ENT-5291
Changelog: None
@vpodzime
Properly report changes when manipulating links
584ae97 
Ticket: ENT-5291
Changelog: None
@vpodzime
Properly report about changes when copying
5b943ab 
Ticket: ENT-5291
Changelog: None
@vpodzime
Properly report changes when editing lines in files
aae236c 
Ticket: ENT-5291
Changelog: None
@vpodzime
Properly report about changes when editing XML files
8d4c1ac 
Also report failures as failures not interruptions.

Ticket: ENT-5291
Changelog: Failures in edit_xml result in promises marked as failed not interrupted
@vpodzime
Properly report changes when rendering contents from templates
4822a54 
Ticket: ENT-5291
Changelog: None
@vpodzime
Properly report and record results of 'files' promises
c27ac00 
Ticket: ENT-5291
Changelog: All changes made by 'files' promises are now reported
@vpodzime
Notes and small fixes
e0afc11 
Ticket: ENT-5291
Changelog: None
@vpodzime
Adapt the 10_files/09_insert_lines/nothing_to_repeat_error.cf test
1fc269b 
With the improved changes reporting the test now produces 'info:'
lines which are expected.

Ticket: ENT-5291
Changelog: None
@vpodzime
Minor fixes for file changes reporting
1240130 
Also adapt the 10_files/02_maintain/changes_update_hashes.cf test
and add comments about why certain classes are (not) defined.

Ticket: ENT-5291
Changelog: None
@vpodzime
Do not define outcome classes for skipped files
489a315 
Ticket: ENT-5291
Changelog: None
@vpodzime
Adapt the 10_files/this_promiser.cf test to better changes reporting
d8c1b4d 
The outcome classes are now defined for the top-level directory
even if 'include_basedir' is 'false'. It makes more sense -- if
there are changes made in the directory, the respective
'_repaired' class is defined. And then it can be used for things
like "Do we need to update the archive/backup of that
directory?".

Ticket: ENT-5291
Changelog: The outcome classes are now defined for the top-level directory when 'include_basedir' is 'false'
@vpodzime
Use 'permissions' instead of 'permission' when (not) changing them
91d0023 
It's a set of permissions.

Ticket: ENT-5291
Changelog: None
@vpodzime
Adapt the 00_basics/02_switches/dry_run_perms_doesnt_lie.cf test
76a149d 
The log messages are now slightly different.

Ticket: ENT-5291
Changelog: None
@vpodzime
Do not overwrite xattrs if they are the same already
955a9ad 
If 'preserve => "true"' is used in a copy_from body and the
xattrs of the source and the destination are the same, the
destination's xattrs should not be overwritten by the source's
xattrs.

Ticket: ENT-5291
Changelog: None
@vpodzime
Properly report about changes in FinishEditContext()
f984491 
Ticket: ENT-5291
Changelog: None
@vpodzime
Report failures in 'fields_edit' promises that don't find a match
9f11404 
Ticket: ENT-5291
Changelog: None
@vpodzime vpodzime force-pushed the master-report_promise_changes branch from f2715a3 to 9f11404 Compare June 10, 2020 12:17
@lgtm-com
Copy link

lgtm-com bot commented Jun 10, 2020

This pull request introduces 98 alerts and fixes 9 when merging 9f11404 into 986bb33 - view on LGTM.com

new alerts:

  • 97 for Pointer argument is dereferenced without checking for NULL
  • 1 for FIXME comment

fixed alerts:

  • 7 for Pointer argument is dereferenced without checking for NULL
  • 1 for Futile conditional
  • 1 for Time-of-check time-of-use filesystem race condition

@vpodzime vpodzime merged commit 51d5deb into cfengine:master Jun 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nickanderson nickanderson nickanderson left review comments

@olehermanse olehermanse olehermanse approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
4 participants
@vpodzime @cf-bottom @nickanderson @olehermanse