Skip to content

Conversation

@issackelly
Copy link
Contributor

Draft, not yet tested. Pushing to see what parts get built/run/tested automatically on branches

@issackelly issackelly force-pushed the issue-10-envvar-apppacktoml branch from 3842cf3 to a9d1261 Compare June 4, 2025 19:15
@ipmb
Copy link
Member

ipmb commented Jun 4, 2025

This looks correct to me. What do you think about factoring this out into a small function?

	filename := "apppack.toml"
	if envFile := os.Getenv("APPPACK_TOML"); envFile != "" {
		filename = envFile
	}

That would be something that we (or your favorite LLM) could add a simple test for. If the tests pass, we can deploy to a canary environment. I can help with that next week.

@issackelly
Copy link
Contributor Author

This looks correct to me. What do you think about factoring this out into a small function?

	filename := "apppack.toml"
	if envFile := os.Getenv("APPPACK_TOML"); envFile != "" {
		filename = envFile
	}

That would be something that we (or your favorite LLM) could add a simple test for. If the tests pass, we can deploy to a canary environment. I can help with that next week.

Great. What about docs or changelogs for the new behavior?

@issackelly issackelly force-pushed the issue-10-envvar-apppacktoml branch from a9d1261 to 5dd49d6 Compare June 10, 2025 00:31
@issackelly issackelly force-pushed the issue-10-envvar-apppacktoml branch from 5dd49d6 to 7a75a5c Compare June 10, 2025 00:51
@issackelly
Copy link
Contributor Author

This MR updated and also:
apppackio/apppack-docs#12

CHANGELOG.md Outdated

## [2.2.0] - 2025-06-12

### Improved
Copy link
Member

Choose a reason for hiding this comment

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

There's a list of headers here at https://keepachangelog.com/en/1.1.0/#how

I would use Added

if appJsonExists && !apppackTomlExists {
// convert app.json to apppack.toml
b.Log().Info().Msg("Converting app.json to apppack.toml")
b.Log().Info().Msg(fmt.Sprintf("Converting %s to %s", "app.json", filename))
Copy link
Member

Choose a reason for hiding this comment

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

why is app.json substituted here?

* Update changelog verb.
* Unset env var after test.
* Fix an unnecessary string formatt
@issackelly
Copy link
Contributor Author

These commits should be squashed before merging.

@ipmb ipmb merged commit 9d48daf into main Sep 19, 2025
7 checks passed
@ipmb ipmb deleted the issue-10-envvar-apppacktoml branch September 19, 2025 21:44
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.

3 participants