#atmos-dev (2024-12)
Discuss atmos core development (golang). If you want to help out, reach out to <@UB2EH889X>
2024-12-02
good for review https://github.com/cloudposse/atmos/pull/815 @Andriy Knysh (Cloud Posse)
2024-12-04
2024-12-05
/github subscribe cloudposse/atmos pulls
:white_check_mark: Subscribed to cloudposse/atmos. This channel will receive notifications for pulls
@haitham911eg should we close https://github.com/cloudposse/atmos/pull/715
yes we should close it
@Dan Miller (Cloud Posse) this was something you had requested, I think.
great! I’ll try it out next time I use workflows
2024-12-06
Hrmmmmm… @Andriy Knysh (Cloud Posse) looks the brew formulae failed on the latest release.
https://github.com/Homebrew/homebrew-core/actions/runs/12200296819/job/34036272201
atmos 1.118.0 #492090
@Igor Rodionov I think we have a race condition, due to the awkward way the workflows are written.
the test workflow calls go autoreleaser
The build workfow calls the homebrew actions https://github.com/cloudposse/atmos/blob/9e0ff68f7683b4adf3b616c0fc64105c986d14dd/.github/workflows/build.yml#L25
The build cannot wait on the tests to finish, thus the homebrew build fails
Maybe it’s related to https://github.com/cloudposse/atmos/pull/721
Which we didn’t merge
I approved it
2024-12-07
@haitham911eg why did we close this? It’s not yet implemented in any other PR
@Erik Osterman (Cloud Posse) Pull request reopened
@Andriy Knysh (Cloud Posse) would love to get this merged before Monday
@haitham911eg please resolve the conflicts
@Andriy Knysh (Cloud Posse) reolved
@haitham911eg I reviewed the PR, it looks good and the new functionality is nice. Please see and address the comments
fixed
@Andriy Knysh (Cloud Posse) please review
@Andriy Knysh (Cloud Posse) good to QA https://github.com/cloudposse/atmos/pull/815
@Vinny did you address the @Erik Osterman (Cloud Posse) comments? If you did, please mark them as resolved or with
its done please check this only https://github.com/cloudposse/atmos/pull/815#issuecomment-2525299250
a few comments, looks good otherwise
thank you
@Vinny please take a look at this:
after we merged https://github.com/cloudposse/atmos/releases/tag/v1.117.0
now if we have a template file stacks/catalog/aws-config/packs.yaml.tmpl with the following content
components:
terraform:
aws-config:
metadata:
component: compliance/aws-config
vars:
conformance_packs:
{{- range $pack := .conformance_packs -}}
{{ $path := index $pack "path" | default $.defaults.path }}
- conformance_pack: {{$path}}/{{$pack.filename}}
name: {{replace ".yaml" "" (toString $pack.filename)}}
{{- $params := index $pack "parameter_overrides" | default dict -}}
{{ if gt (len $params) 0 }}
parameter_overrides:
{{- range $key, $value := $params }}
{{ $key }}: {{ $value }}
{{- end -}}
{{ else }}
parameter_overrides: {}
{{ end -}}
{{ end -}}
this is not a valid YAML
when executing atmos validate stacks, it now throws an error
yaml: line 9: did not find expected node content
(which is expected since the file is not a valid YAML)
before the change in the v1.117.0 release, we did not treat the template files as YAML files and atmos validate stacks returned a success (we only selected the files with extensions .yaml and .yaml for validation)
now we treat all of the files as Atmos stack manifests and fail if a file is not a valid YAML as the above
Ok this raises an important edge case: Should .yaml.tmpl files with errors be ignored during validation, allowing the process to continue? If we choose to ignore these files, we need to carefully consider the implications of this decision
They are invalid yaml files so it doesn’t make sense to validate
We have a separate task to render then validate
But since we broke existing behavior, and this is widely adopted, we don’t have time to do anything else than re-ignore them like before implementing it a better way
we broke it for one of the clients, and i asked them to change the file extension from .tmpl to .templ. - this extension is not validated automatically
@Vinny please confirm that it’s working
@Andriy Knysh (Cloud Posse) PR to address this issue https://github.com/cloudposse/atmos/pull/830
@Vinny a few comments, please review
@Andriy Knysh (Cloud Posse)
@Vinny please review a few more comments
2024-12-08
/github unsubscribe cloudposse/atmos
Unsubscribed from cloudposse/atmos
2024-12-09
@Vinny please also resolve the conflicts here https://github.com/cloudposse/atmos/pull/815
Hi Team, this PR is good to review
@Pulak Kanti Bhowmick if there is a tty attached it should be a polished table.
See section for rendering tables
If no tty attached , for example, piping through grep, then it should display the way you have it now
okay, got it. updating
@Erik Osterman (Cloud Posse) this is the latest image
btw, this is a sample. I might update color and add the values
Yes, this is on the right track
Let’s use the purple colors like in the lipgloss example
Also, we still need to add the filters for real/abstract components
2024-12-10
https://github.com/cloudposse/atmos/pull/815 this is good to review
@Andriy Knysh (Cloud Posse)
I am starting pushing charmbracellet everywhere I visit while working on the issues, so we can have better UI btw
2024-12-11
set the channel topic: Discuss atmos core development (golang). If you want to help out, reach out to @Erik Osterman (Cloud Posse)
@Andriy Knysh (Cloud Posse) so here https://linear.app/cloudposse/issue/DEV-2785/running-atmos-workflow-incorrectly-produces-useless-usage I noticed running things like atmos workflow list throws an error because theres no stacks path
so I was thinking in something more user friendly like
Workflows directory not found at '/Users/viniciuscardoso/Developer/atmos/stacks/workflows'
To set up workflows:
1. Create a workflows directory at '/Users/viniciuscardoso/Developer/atmos/stacks/workflows'
2. Add your workflow YAML files in this directory
3. Configure the workflows path in your atmos.yaml:
workflows:
basePath: stacks/workflows
For more information, visit: <https://atmos.tools/cli/commands/workflow>
of course I am going to remove those uselless error msgs, but I am also think in ways to improve that btw we can improve this output using charmbracelet
2024-12-12
2024-12-13
It’s not urgent, but can I get a review on this PR that implements a basic pager for the atmos docs command?
https://github.com/cloudposse/atmos/pull/848
@Andriy Knysh (Cloud Posse) hi, have pushed the PR, would appreciate if you could please take a look when you have a minute. https://github.com/cloudposse/atmos/pull/844 @Erik Osterman (Cloud Posse) FYI
@Andriy Knysh (Cloud Posse) hi, noticed Confirm auto-merge button appeared at the bottom of the PR. Should I press it or you guys will take care of if?
@Andriy Knysh (Cloud Posse) has been consumed by client work this week, so reviews have ground to a halt
i’ll review all of this tomorrow (still need to finish some client work)
2024-12-17
@Andriy Knysh (Cloud Posse) cannot reproduce the problem you raised: https://github.com/cloudposse/atmos/pull/768
We also added a test for this that is passing. Can you try again?
https://linear.app/cloudposse/issue/DEV-2322/help-not-showing-for-custom-commands-when-using-the-nested-command Do we know who raised this ticket. We would require reproduction steps.
Whenever someone creates a bug ticket please mention the steps, This avoids back and forth.
- Steps
- Screenshots
- Expected behavior These 3 are a must in a bug ticket
@Andriy Knysh (Cloud Posse) do you know if there are any of these unsupported commands we actually support?
@Shubham Tholiya is helping us clean up atmos terraform --help
do you know if there are any of these unsupported commands we actually support?
atmos terrafrom xxx (and atmos helmfile xxx) are special cases in the sense that we did not hardcode any of the native sub-commands in Cobra
all of that is “transparent” and just sent to the native terraform/tofu/helmfile CLI
that made the code more “convoluted”, but allows us to support any new subcommands they release w/o updating the Atmos code
I would say we support many (if not all) of those subcommands, maybe except the ones with a diff invocation sysntax
and b/c of that “transparency”, we can’t use the native Cobra help functionality
we had to implement atmos terrafrom --help by ourselves (which is the point of this discussion)
It’s less important that we support any new command in terraform
What is most useful is a cli help menu that is consistent with the product
So we are adding the commands to the cobra menu and not trying to be clever
So I say, unless we have tested the command and verified it works, it should be unsupported
Rather than “supported” and broken
atmos terraform output <component> -s <stack> has the same syntax as atmos terraforn apply, and is used all the time
atmos terraform force-unlock <component> -s <stack> --force <lock ID> - used all the time
atmos terraform import <component> -s <stack> <ADDRESS> <ID> - used all the time
atmos terraform show <component> -s <stack> - works and has the same syntax as atmos terraform plan
the rest are not used often (if at all), we need to confirm them
Ok, great
@Shubham Tholiya let’s verify these flags and usage are available with help
atmos terraform validate <component> -s <stack> should work too
cool thanks for letting me know
Also, I see we’re missing deploy
And shell subcommands
i guess that might be native to atmos terraform command right? because in terraform i don’t see it
atmos terraform deploy <component> -s <stack>
atmos terraform shell <component> -s <stack>
these are atmos command, not present in terraform
These commands should go under the top section, of atmos-native commands like generate
2024-12-18
Hey anyone found this issue the long description for terraform does not get printed. i added .Long in template and now every command shows description twice for all other commands except terraform
just noticed this has to do something with duplicated help strategy that we follow we have exec/help.go and templater.go so when migrating there is some missing change.
But for long term i would recommend cobra based command help
@Andriy Knysh (Cloud Posse) Wdyt?
@Erik Osterman (Cloud Posse) i would recommend we migrate all commands first
This will be
- Consistent help UX for customer
- Consistent dev Experience and less confusion for new Dev to implement the help
I would prefer we find some way to use the cobra help menu for everything even if it means duplication
And by duplication, I mean duplication of effort that we have to copy everything into cobra
2024-12-19
@Andriy Knysh (Cloud Posse) hi, if it wouldn’t be too much trouble, could you please take a look when you have a minute? https://github.com/cloudposse/atmos/pull/866
2024-12-20
:alert: main is currently broken. It was accidentally broken in https://github.com/cloudposse/atmos/pull/856, which broke Windows support because we had no tests run on Windows.
We merged https://github.com/cloudposse/atmos/pull/875, which adds windows & macos tests, so until we resolve the breaking change in 856, windows tests are failing.
@Shubham Tholiya is looking into why the tests are failing
heads up @Matt Calhoun
let me know if you guys need help
@Matt Calhoun is also going to take a quick look.
Also, it’s worth saying, until this is fixed, every release include 1.128 and later is broken.
Not sure if its worth noticing but any –help is not working either now. And I’m on a win machine.
Oh, wonderful, you’re on windows~!
You can probably help us fix this problem
@Matt Calhoun opened a PR that fixed one of the problems.
Maybe you can expand this to the rest
But please confirm if. you can work on this now, otherwise @Shubham Tholiya was going to work on it. I’m not sure if he’s on windows.
@Matt Calhoun is no longer working on this problem. So not sure who can pick it up fastest. We need to get this fixed ASAP.
The fix is something like
// Conver the pattern to use / as the separatoratorator regardless of platform
allOsSafePattern := filepath.ToSlash(pattern)
base, cleanPattern := doublestar.SplitPattern(allOsSafePattern)
f := os.DirFS(base)
and we need to do that probably in more places.
But good to know we have one more issue
Ok, thanks @Shubham Tholiya
@Shubham Tholiya there is another problem I tried to fix, but won’t get back to this before Monday.
The problem is that in the matrix, when we run on windows, it doesn’t find atmos.exe. I am not to versed on windows, and tried to fix it in this PR. I think the general techniques are here, but still needs a tweak to get it working.
Hi, just a quck status update about the –help issue on Windows I reported above. It has gone once I rolled back the history of commits to this one 2394a9d fix tests (#868). On this commit as HEAD –help on Win works fine. @Andriy Knysh (Cloud Posse) fyi
It should have started on 856 (1.128)
Thanks for the update @Aaron Bidderman
@Aaron Bidderman DM’d me some other info
@Aaron Bidderman
Hi @Erik Osterman (Cloud Posse) Im not sure if its a windows problem at least with the –help. I wanted to rollback a few commits to see if the –help starts work and when and did $ git log --oneline
80058a8 (HEAD -> main, origin/main, origin/add-vars-parameter, origin/HEAD) refactor loading and checking of atmos config (#869) fe637fa rename CliConfiguration to AtmosConfiguration (#867) 2394a9d fix tests (#868) 05606ad (upstream/main) atmos read storage interface (#865) 1defdce (tag: v1.130.0) package manager for vendor pull (#768) c4fe769 (tag: v1.129.0) Improve
!terraform.outputAtmos YAML function. Implementstaticremote state backend for!terraform.outputandatmos.Componentfunctions (#863) 2051592 (tag: v1.128.0) Replace path.Join with filepath.Join (#856) 74f3931 Mergify (#862) b205a0d (tag: v1.127.0) Implement Atmos Version Check Configuration (#844) e9a610e (tag: v1.126.0) add support for terraform version (#846) 11ec388 Add support for helmfile version command (#854) 58b46f5 Append new lines to end of json files before writing (#849) ed93da1 (tag: v1.125.0) Exclude abstract components from list (#851) 5681e54 (tag: v1.124.0) feat: add pagination and searching toatmos docscommand (#848) c0e7054 (tag: v1.123.1) Fix terraform plan file should not be deleted (#855) 22c9b28 (tag: v1.123.0) Dynamic width terminal calculation for help (#815) b144ba2 Fix deployments inactivate (#843) 1bf5dc2 Update Atmos stack manifest JSON schema. Fix docs (#842) 36a595e Fix deployments overrides (#841) abddf14 (tag: v1.122.0) Ignore tmpl files to be validated (#830) bf867be Fix preview deployment (#839) 067a99f Refactor preview deployments workflows (#836)
So the - 2051592 (tag: v1.128.0) Replace path.Join with filepath.Join (#856) is still there with me but any changes as adding pointers to config schema/renaming it are gone and –help works. So I guess - refactor loading and checking of atmos config (#869) - has issues and as a side note adding pointers to config is major change that requires retesting everything. Please disregard as I caould be totally wrong and didnt want to point fingers to anyone.
@Vinny @Aaron Bidderman @Shubham Tholiya please hold off on working on this one any more
So part of the problem is that the paths are being returned as absolute paths on Windows and relative on linux
We should get them all as relative
This PR is almost ready. There’s only one failing test and I can’t figure it out.
2024-12-21
2024-12-22
2024-12-23
2024-12-24
@Andriy Knysh (Cloud Posse) , @haitham911eg helped me fix the tests: https://github.com/cloudposse/atmos/pull/887
This should restore atmos behavior and ensure windows is tested moving forward
2024-12-26
@Vinny would be great to get your glamour inprovements ready. I think we need it to finish the work @Shubham Tholiya is doing on the terraform help pages.
We have more improvements?
@Vinny would be great if we could collaborate
https://linear.app/cloudposse/issue/DEV-2722/format-examples-output-by-atmos-help-command-in-color
Based on this issue. I or you can incorporate that in the examples section of the template. This way we would not change the structure we are looking for yet work collaboratively.
My pr that we should be updating is: https://github.com/cloudposse/atmos/pull/857
Let me know if you need some help
good point I think theres a bunch of things solved on the glamour improvements that @Shubham Tholiya can reuse
2024-12-27
Just putting things here so that we do not re work everytime on help:
We have a pr: https://github.com/cloudposse/atmos/pull/857
This pr will try to depricate help.go in favour of supporting native cobra help.
Please do not add/edit help.go .
If required contribute to the pr above
We would have followup pr’s ofcourse that i will update as and when required.
thanks for the PR @Shubham Tholiya
i left some comments
I updated the pr based on your comment. A follow up pr would be required for sure. There are still things i am sorting up like
There are two places where we set the help func This should not be the case because it makes the cobra help itself inconsistent.
Post fixing that i will have to do the same thing i did with terraform for helm And then eventually have it our way.
Just to make @Erik Osterman (Cloud Posse) aware of this. A lot of help are so mixed up that is the reason we are moving in phases This will help us to test effectively and avoid breaking anything
thank you, i’ll review it
yes, help was convoluted, because we started with one way , then manually added terraform and helmfile help
2024-12-28
Just wanted to understand this I observe that viper and cobra are not binded as per recommendations While i do understand why it might have been that way. I would recommend let us change that so that we remove a lot of code
Wdyt @Andriy Knysh (Cloud Posse) cc: @Erik Osterman (Cloud Posse)
please explain in more details how they are not integrated and how we can improve the code
note that you don’t need to use Viper with Cobra, Cobra can be used separately
in the current code, we are using Viper to merge the CLI configurations loaded from different places
So what i noticed is we manually check if a flag was set and the modify the config. If i remember the viper and cobra correctly viper can directly handle this binding
Unless this is something we already have planned to remove and i am looking at some old code
ohh sorry it is cobra+viper or viper+native that automatically handles this. Not sure if we want to do that or we already droped that for some reasons
a snippet from chat gpt
We do want to use viper, but I have not written the task
We want to convert our handling of atmos.yml to use viper
I probably won’t get to writing task until Monday
we already using Viper to handle atmos.yaml and merge the config from many files if they are found
Ok… this: https://github.com/cloudposse/atmos/blob/main/pkg/config/utils.go#L188-L387 Could be handled by viper viper help attached for reference:
~We can even standardize env and arguments in a future Major version so it is intutive for the user to know the env and flag maybe~i class=”em em-slightly_smiling_face”></i>
Not diverging the topic avoid this
yes, you are right, we can use Viper to handle the ENV vars (this will need to be tested for all scenarios)
2024-12-30
@haitham911eg please check https://github.com/cloudposse/atmos/issues/832#issuecomment-2565902028
2024-12-31
Hey team this is an important observation please check my comment: https://github.com/cloudposse/atmos/pull/896/files#r1899825937 @Andriy Knysh (Cloud Posse) or @Erik Osterman (Cloud Posse) might know if there are specific reasons for this (if they are aware of this) Currently i stand on the side that this should not work the way it is. Unless someone convinces me of the reasons these special file configs exists.
Correct, changing the destination of the logs, should have no bearing on what is emitted. That should be strictly controlled by the log level.
I have also noticed that setting the ATMOS_LOGS_LEVEL environment variable, or passing the —logs-level flag seems to have no bearing
Maybe it’s because we usually set the file to stderr
@Andriy Knysh (Cloud Posse)
I guess we should create a medium - high priority task to fix log level
This is one of the basic thing we should not mess up. Ofcourse we can discuss the behavior before we standardize something
Also just curioius. Do we have a document subscription like Norton or Quip where we propose and discuss stuff? or it is github issue? I am fine with github issue as well just wanted to know what we prefer
atmos logs level env variable has been working correctly
i’ll check your comments in a few
We have another task for @Vinny to implement the charm logger. We can do the work as part of that.
if you have trouble with that, let’s sync tomorrow with you, Erik and myself to discuss
note that the two places that you pointed out
<https://github.com/cloudposse/atmos/blob/main/pkg/logger/logger.go#L61-L98>
<https://github.com/cloudposse/atmos/blob/main/pkg/utils/log_utils.go#L106-L143>
the second one is a new way of login, the second one is a very old code
(they will need to be consolidated eventually, all the old places need to be switched to use the new Logger)
When I discussed with @Matt Calhoun we decided to do a 2 phased approach. Update the current logging functions to use the charm logger, and everything new call the charm logger directly.
@Andriy Knysh (Cloud Posse)
I believe my initial concern was misunderstood, so I clarified it here: https://github.com/cloudposse/atmos/pull/896/files#r1900454315 Since we’re moving to a new logger, this issue might resolve itself, so it may not need further discussion. Just sharing this in case it’s helpful.
@Shubham Tholiya i commented on your comment
We have the correct handling of log levels and log files now via the top-level functions LogTrace, LogDebug, LogInfo (which are called from the Go code), and the internal (not exported) function log (which handles the log files and file descriptors
this is how the rest of the code calls the LogXXX functions
// func "a" logs at the log level Info, meaning the messages will be logged when the configured log level is Info, Debug, Trace, etc.
func a() {
LogInfo(..., "Starting doing something")
.....
LogInfo(..., "Finished doing something")
}
// func "b" logs at the log level Trace, meaning the messages will be logged only when the configured log level is Trace
func b() {
LogTrace(..., "Starting doing something")
.....
LogTrace(..., "Finished doing something")
}
only the code that actually logs (e.g the a and b funcs above) knows at what log level it wants the messages to be logged