envoyproxy / envoyproxy/toolshed

`envoy.base.utils`: `write_version` removes `changelogs/current/PLACEHOLDER.txt`

Open
#4,642 0 comments 1 reaction 2 assignees Claimed by @phlax View on GitHub
bug
Dominant language
Python
Stars
12
Forks
24
Avg merge
6h 37m
Merged PRs (30d)
92

Description

## Problem

In `py/envoy.base.utils/envoy/base/utils/abstract/project/changelog.py`, the `write_version` method for the entries-layout branch does:

```python
def write_version(self, version: _version.Version) -> None:
...
if self.entries_layout:
data = self.changelog_class.get_data_from_entries(
self.current_dir_path)
data["date"] = self.datestamp
version_file.write_text(self.dump_yaml(data))
shutil.rmtree(self.current_dir_path)
self.current_dir_path.mkdir()
...
```

`shutil.rmtree(self.current_dir_path)` deletes every file in `changelogs/current/`, including the `PLACEHOLDER.txt` that the envoy repo keeps so that the otherwise-empty directory stays tracked by git. The follow-up `mkdir()` re-creates the directory, but it is empty, so git records a deletion of `PLACEHOLDER.txt` and nothing replaces it.

`write_current()` has the same problem on its entries-layout branch — it only ensures the directory exists.

Symptom: the recently published `Dev v1.35.13` commit (https://github.com/envoyproxy/envoy/commit/99598ec71215cc75c25ffddc178c17569147576e) removed `changelogs/current/PLACEHOLDER.txt` (see https://github.com/envoyproxy/envoy/pull/45571), causing Bazel `//changelogs:changelogs` (which globs `current/PLACEHOLDER.txt`) and the Envoy/Prechecks workflow to fail.

The `release/v1.35` envoy `changelogs/BUILD` filegroup explicitly references the placeholder:

```python
filegroup(
name = "changelogs",
srcs = glob([
"*.*.*.yaml",
"current/**/*.rst",
"current/PLACEHOLDER.txt",
]) + ["changelogs.yaml"],
visibility = ["//visibility:public"],
)
```

so the file *must* stay present in the dev commit even when there are no per-entry RST files.

## Proposed fix

In `py/envoy.base.utils/envoy/base/utils/abstract/project/changelog.py`:

1. Add a constant:

```python
CHANGELOG_CURRENT_PLACEHOLDER = "PLACEHOLDER.txt"
```

2. Add a property on `AChangelogs`:

```python
@property
def current_placeholder_path(self) -> pathlib.Path:
return self.current_dir_path.joinpath(CHANGELOG_CURRENT_PLACEHOLDER)
```

3. After `write_version` re-creates the empty dir (in the entries layout branch) `touch` the placeholder so git keeps tracking the directory:

```python
if self.entries_layout:
data = self.changelog_class.get_data_from_entries(
self.current_dir_path)
data["date"] = self.datestamp
version_file.write_text(self.dump_yaml(data))
shutil.rmtree(self.current_dir_path)
self.current_dir_path.mkdir()
self.current_placeholder_path.touch()
```

4. Do the same in `write_current()` on its entries-layout branch so projects that start the dev workflow without an existing placeholder also end up with one:

```python
def write_current(self) -> None:
if self.entries_layout:
self.current_dir_path.mkdir(parents=True, exist_ok=True)
self.current_placeholder_path.touch()
else:
...
```

## Tests to update / add

In `py/envoy.base.utils/tests/test_abstract_project_changelogs.py`:

- Add `test_abstract_changelogs_current_placeholder_path` covering the new property.
- Update `test_abstract_changelogs_write_current` to also patch `AChangelogs.current_placeholder_path` and assert `placeholder.touch` is called on the entries-layout branch and not called on the legacy branch.
- Update `test_abstract_changelogs_write_version` similarly: on the entries-layout branch `placeholder.touch` is called, on the legacy branch it isn't.
- Update `test_abstract_changelogs_write_version_entries_parse_error` to assert `placeholder.touch` is *not* called when `get_data_from_entries` raises.
- Update the end-to-end `test_abstract_changelog_entries_layout_no_current_yaml` test to assert that after `write_version` runs, `changelogs.current_placeholder_path` exists, is a file, is empty, and is the only entry left in `current_dir_path`.

## Verification

A small reproduction in a temp dir confirms the fix:

```text
BEFORE write_version:
changelogs/current/PLACEHOLDER.txt
changelogs/current/bug_fixes
changelogs/current/bug_fixes/jwt__foo.rst

AFTER write_version (fixed):
changelogs/current/PLACEHOLDER.txt

AFTER write_current (fixed):
changelogs/current/PLACEHOLDER.txt
```

Without the fix the `AFTER` cases are empty directories, which is exactly what happened in https://github.com/envoyproxy/envoy/pull/45571.

## Acceptance criteria

- New `CHANGELOG_CURRENT_PLACEHOLDER` constant + `current_placeholder_path` property.
- `write_current` and `write_version` re-create the placeholder on the entries-layout branch.
- Tests above are added/updated.
- `bazel run //tools/check:check_format` / pytest still pass.

cc @phlax

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.