Repository: dotenv-linter/dotenv-linter
Pull request #272 (2020-08-21 01:28:32)

Implement Backup Feature
Hey All, I had some questions/comments about this PR. The basic functionality is in the PR to create a copy of a file via the -c/--copy flag, though.

Questions:
1) I just used a Unix epoch time timestamp. We could add a more nicely formatted one, but it seems likely we'd need to add a crate (I'm not sure?), and I don't know if that's wanted or needed. The Unix timestamps still allow the copies to easily be sorted by most recent.

2) Should copying the file be automatic any time there are fixes?
    1) If it should be automatic, should there be an opt-out flag? If so, what flag? -n//--no-copy?
    2) If users should always specify a flag to create a copy, should we prompt them that their file will get overwritten?

- [x] This PR has been added to [CHANGELOG.md](https://github.com/dotenv-linter/dotenv-linter/blob/master/CHANGELOG.md) (at the top of the list);
- [x] Tests for the changes have been added (for bug fixes / features);
- [ ] Docs have been added / updated (for bug fixes / features).

baile320: Just an FYI - I haven't forgotten about this, just been busy. I think I'll have time do make the changes later today.
baile320: I believe this should be ready for review now. Changes made since discussion:

1. Renamed feature from Copy to Backup (changed arg to "no-backup", --no-backup, and no short name)
2. Using fs::copy instead of fs::write to create the file.
3. Inform users of the file created after creating it.
4. Only back up file if the .env file is being modified
5. Added the backup files to .gitignore as well (*.env_*)
baile320: I'm not sure why the ```CI / Test and coverage / Linux (pull_request) Failing after 45s — Test and coverage / Linux``` is failing, is this something I need to look into? I see a couple other PRs where it failed as well.
evgeniy-r: > I'm not sure why the CI / Test and coverage / Linux (pull_request) Failing after 45s — Test and coverage / Linux is failing, is this something I need to look into?

This is due to a change in the output:
https://github.com/dotenv-linter/dotenv-linter/pull/272/checks?check_run_id=1047857184#step:9:210



mgrachev: > This is due to a change in the output:
> https://github.com/dotenv-linter/dotenv-linter/pull/272/checks?check_run_id=1047857184#step:9:210

Please also update all integration tests and write another one to cover the `--no-backup` flag.

codecov-commenter: # [Codecov](https://codecov.io/gh/dotenv-linter/dotenv-linter/pull/272?src=pr&el=h1) Report
> Merging [#272](https://codecov.io/gh/dotenv-linter/dotenv-linter/pull/272?src=pr&el=desc) into [master](https://codecov.io/gh/dotenv-linter/dotenv-linter/commit/df1b0b2ba2d44e7b65e17b83c1e852ee399814c8?el=desc) will **decrease** coverage by `0.18%`.
> The diff coverage is `86.66%`.

[![Impacted file tree graph](https://codecov.io/gh/dotenv-linter/dotenv-linter/pull/272/graphs/tree.svg?width=650&height=150&src=pr&token=wQybePfHI1)](https://codecov.io/gh/dotenv-linter/dotenv-linter/pull/272?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #272      +/-   ##
==========================================
- Coverage   97.64%   97.46%   -0.19%     
==========================================
  Files          31       31              
  Lines        2633     2679      +46     
==========================================
+ Hits         2571     2611      +40     
- Misses         62       68       +6     
```


| [Impacted Files](https://codecov.io/gh/dotenv-linter/dotenv-linter/pull/272?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [src/lib.rs](https://codecov.io/gh/dotenv-linter/dotenv-linter/pull/272/diff?src=pr&el=tree#diff-c3JjL2xpYi5ycw==) | `89.85% <40.00%> (-3.90%)` | :arrow_down: |
| [src/fs\_utils.rs](https://codecov.io/gh/dotenv-linter/dotenv-linter/pull/272/diff?src=pr&el=tree#diff-c3JjL2ZzX3V0aWxzLnJz) | `97.14% <92.30%> (-2.86%)` | :arrow_down: |
| [src/main.rs](https://codecov.io/gh/dotenv-linter/dotenv-linter/pull/272/diff?src=pr&el=tree#diff-c3JjL21haW4ucnM=) | `91.48% <100.00%> (+0.18%)` | :arrow_up: |
| [src/checks/unordered\_key.rs](https://codecov.io/gh/dotenv-linter/dotenv-linter/pull/272/diff?src=pr&el=tree#diff-c3JjL2NoZWNrcy91bm9yZGVyZWRfa2V5LnJz) | `100.00% <0.00%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/dotenv-linter/dotenv-linter/pull/272?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/dotenv-linter/dotenv-linter/pull/272?src=pr&el=footer). Last update [df1b0b2...fa2ba12](https://codecov.io/gh/dotenv-linter/dotenv-linter/pull/272?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

baile320: > Please also update all integration tests and write another one to cover the `--no-backup` flag.

Still need to complete this task, will get to it later today and post a reply when completed
baile320: hmm... i'm not sure how much detail I should be checking when a backup is created in an integration test. since the file is created with a timestamp, i'm not sure how to know when testing what that timestamp would be? I could just check for a wildcard file or something if that's necessary? I feel a little stuck on testing these fs components.

but in the unit test i already do make sure the filepath & backup filepath are different, and that the backup filepath has the original environment.
mgrachev: > hmm... i'm not sure how much detail I should be checking when a backup is created in an integration test. since the file is created with a timestamp, i'm not sure how to know when testing what that timestamp would be? I could just check for a wildcard file or something if that's necessary? I feel a little stuck on testing these fs components.
> 
> but in the unit test i already do make sure the filepath & backup filepath are different, and that the backup filepath has the original environment.

In my opinion, in the intragration test you should create a file with some problems, for example: `foo=bar`.
Then, you should run `dotenv-linter` with the `-f` flag and check that:
1. The content of the `.env` file doesn't equal `foo=bar`;
2. The content of the `backup` file equal `foo=bar`;

I think that is all.
baile320: Hm. I think having a separate function for a single test is probably overkill as well, but I think I need a testing function that has some different functionality than the current ones. 

I think I need a test function that will let me 1) supply arguments 2) not close/delete TestDir when done and 3) doesn't require me to supply the expected output. The reasons for those are 1) I changed the other tests to have ```-f --no-backup``` (I think this is OK, it preserves the functionality of the tests that existed prior to adding this feature),  2) I need to be able to the examine the TestDir for the backup file after running, and 3) I don't know what the expected output will be because we're generating a dynamic timestamp every time we run, and I don't see an easy way of mocking that value.

I'm open to suggestions on any of these points, especially the last one.
mgrachev: > Hm. I think having a separate function for a single test is probably overkill as well, but I think I need a testing function that has some different functionality than the current ones.
> 
> I think I need a test function that will let me
> 
> 1. supply arguments
> 2. not close/delete TestDir when done, and doesn't take ownership
> 3. doesn't require me to supply the expected output.
> 
> The reasons for those are:
> 
> 1. I changed the other tests to have `-f --no-backup` (I think this is OK, it basically acts to preserve the functionality of the tests that existed prior to adding this feature),
> 2. I need to be able to the examine the TestDir for the backup file after running
> 3. I don't know what the expected output will be because we're generating a dynamic timestamp every time we run, and I don't see an easy way of mocking that value.
> 
> I'm open to suggestions on any of these points, especially the last one.

👌 Let it be 🙂

Patch:
diff --git a/.gitignore b/.gitignore
index 85ca923..b3cc8aa 100644
--- a/.gitignore
+++ b/.gitignore
@@ -10,3 +10,4 @@
 
 # Environment Files
 *.env
+*.env_*
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 7740f33..6c1b793 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -6,6 +6,7 @@ # Changelog
 
 ## [Unreleased]
 ### 🚀 Added
+- Add backup feature and `--no-backup` flag for fixers [#272](https://github.com/dotenv-linter/dotenv-linter/pull/272) ([@baile320](https://github.com/baile320))
 - Add fixer: ExtraBlankLineFixer [#260](https://github.com/dotenv-linter/dotenv-linter/pull/260) ([@diggymo](https://github.com/diggymo))
 - Add fixer: DuplicatedKeyFixer [#270](https://github.com/dotenv-linter/dotenv-linter/pull/270) ([@utter-step](https://github.com/utter-step))
 - Add fixer: LeadingCharacter [#259](https://github.com/dotenv-linter/dotenv-linter/pull/259) ([@baile320](https://github.com/baile320))
diff --git a/src/fs_utils.rs b/src/fs_utils.rs
index 38d61ae..370b9ed 100644
--- a/src/fs_utils.rs
+++ b/src/fs_utils.rs
@@ -1,7 +1,9 @@
-use crate::common::LineEntry;
-use std::fs::File;
+use crate::common::{FileEntry, LineEntry};
+use std::error::Error;
+use std::fs::{copy, File};
 use std::io::{self, Write};
 use std::path::PathBuf;
+use std::time::SystemTime;
 
 /// For the Windows platform, we need to remove the UNC prefix.
 #[cfg(windows)]
@@ -47,6 +49,20 @@ pub fn write_file(path: &PathBuf, lines: Vec<LineEntry>) -> io::Result<()> {
     Ok(())
 }
 
+pub fn backup_file(fe: &FileEntry) -> Result<PathBuf, Box<dyn Error>> {
+    let timestamp = SystemTime::now()
+        .duration_since(SystemTime::UNIX_EPOCH)?
+        .as_secs();
+
+    let mut new_path = fe.path.to_owned();
+    new_path.set_file_name(format!("{}_{}", &fe.file_name, timestamp));
+
+    match copy(&fe.path, &new_path) {
+        Ok(_) => Ok(new_path),
+        Err(e) => Err(Box::new(e)),
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
@@ -168,4 +184,52 @@ fn write_file_test() {
 
         dir.close().expect("temp dir deleted");
     }
+
+    #[test]
+    fn backup_file_test() {
+        let file_name = String::from(".env");
+        let dir = tempfile::tempdir().expect("create temp dir");
+        let path = dir.path().join(&file_name);
+
+        let fe = FileEntry {
+            path,
+            file_name,
+            total_lines: 3,
+        };
+
+        let lines = vec![
+            LineEntry {
+                number: 1,
+                file: fe.clone(),
+                raw_string: String::from("A=B"),
+            },
+            LineEntry {
+                number: 2,
+                file: fe.clone(),
+                raw_string: String::from("Z=Y"),
+            },
+            LineEntry {
+                number: 3,
+                file: fe.clone(),
+                raw_string: String::from("\n"),
+            },
+        ];
+
+        if write_file(&fe.path, lines).is_ok() {
+            match backup_file(&fe) {
+                Ok(path) => {
+                    assert_eq!(
+                        b"A=B\nZ=Y\n",
+                        fs::read(path.as_path()).expect("file read").as_slice()
+                    );
+                    assert_ne!(path, fe.path);
+                }
+                Err(_) => panic!("could not copy file - test failed"),
+            }
+        } else {
+            panic!("could not write file - test failed")
+        }
+
+        dir.close().expect("temp dir deleted");
+    }
 }
diff --git a/src/lib.rs b/src/lib.rs
index 90239cc..0dfc6ef 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -52,8 +52,17 @@ pub fn run(args: &clap::ArgMatches, current_dir: &PathBuf) -> Result<Vec<Warning
 
         let mut lines = get_line_entries(&fe, strs);
 
+        // run fixers & write results to file
         let mut result = checks::run(&lines, &skip_checks);
         if is_fix && fixes::run(&mut result, &mut lines) > 0 {
+            // create backup copy unless user specifies not to
+            let should_backup = !args.is_present("no-backup");
+            if should_backup {
+                let backup_file = fs_utils::backup_file(&fe)?;
+                println!("Original file was backed up to: {:?}\n", backup_file);
+            }
+
+            // write corrected file
             fs_utils::write_file(&fe.path, lines)?;
         }
 
diff --git a/src/main.rs b/src/main.rs
index 066bcb9..28bab04 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -110,6 +110,11 @@ fn get_args(current_dir: &OsStr) -> clap::ArgMatches {
                 .long("fix")
                 .help("Automatically fixes warnings if possible"),
         )
+        .arg(
+            Arg::with_name("no-backup")
+                .long("no-backup")
+                .help("Prevents .env files from being backed up when modified by -f/--fix"),
+        )
         .arg(
             Arg::with_name("quiet")
                 .short("q")


Test patch:
diff --git a/tests/common/mod.rs b/tests/common/mod.rs
index 39dac79..2c348a1 100644
--- a/tests/common/mod.rs
+++ b/tests/common/mod.rs
@@ -121,7 +121,7 @@ pub fn test_command_fix_success(&self, expected_output: String) {
         let mut cmd = Self::init_cmd();
         let canonical_current_dir = canonicalize(&self.current_dir).expect("canonical current dir");
         cmd.current_dir(&canonical_current_dir)
-            .args(&["-f"])
+            .args(&["-f", "--no-backup"])
             .assert()
             .success()
             .stdout(expected_output);
@@ -132,13 +132,30 @@ pub fn test_command_fix_fail(&self, expected_output: String) {
         let mut cmd = Self::init_cmd();
         let canonical_current_dir = canonicalize(&self.current_dir).expect("canonical current dir");
         cmd.current_dir(&canonical_current_dir)
-            .args(&["-f"])
+            .args(&["-f", "--no-backup"])
             .assert()
             .failure()
             .code(1)
             .stdout(expected_output);
     }
 
+    /// Run the default CLI binary, with command line arguments,
+    /// in this TestDir and check it succeeds.
+    ///
+    /// This method does NOT remove TestDir when finished
+    pub fn test_command_success_with_args_without_closing<I, S>(&self, args: I)
+    where
+        I: IntoIterator<Item = S>,
+        S: AsRef<OsStr>,
+    {
+        let mut cmd = Self::init_cmd();
+        let canonical_current_dir = canonicalize(&self.current_dir).expect("canonical current dir");
+        cmd.current_dir(&canonical_current_dir)
+            .args(args)
+            .assert()
+            .success();
+    }
+
     fn init_cmd() -> Command {
         Command::cargo_bin(env!("CARGO_PKG_NAME")).expect("command from binary name")
     }
diff --git a/tests/flags/backup.rs b/tests/flags/backup.rs
new file mode 100644
index 0000000..7ec8594
--- /dev/null
+++ b/tests/flags/backup.rs
@@ -0,0 +1,27 @@
+use crate::common::TestDir;
+use std::fs;
+
+#[test]
+fn output_backup_file() {
+    // test that a backup file has been created
+    let testdir = TestDir::new();
+    let content = "foo=bar\n";
+    let testfile = testdir.create_testfile(".env", content);
+    let args = &[testfile.as_str(), "-f"];
+
+    testdir.test_command_success_with_args_without_closing(args);
+
+    let backup_file = fs::read_dir(&testdir.as_str())
+        .expect("read dir")
+        .filter_map(|e| e.ok())
+        .filter(|e| e.path().as_os_str() != testfile.as_str())
+        .last()
+        .expect("get backup file");
+
+    let backup_contents = fs::read_to_string(backup_file.path()).expect("read backup file");
+
+    assert_eq!(backup_contents, content);
+    assert_eq!(testfile.contents(), "FOO=bar\n");
+
+    testdir.close()
+}
diff --git a/tests/flags/mod.rs b/tests/flags/mod.rs
index 2420ec8..6205a5a 100644
--- a/tests/flags/mod.rs
+++ b/tests/flags/mod.rs
@@ -1,2 +1,3 @@
+mod backup;
 mod quiet;
 mod recursive;