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

Add support for cmd/tlm partials #170 #173

Merged
merged 10 commits into from
Jul 17, 2015
Merged

Conversation

jmthomas
Copy link
Contributor

This isn't ready for merge yet (I'd like to support parameters) but check it out and see what you think.

@jmthomas
Copy link
Contributor Author

I tried to implement parameters as close as possible to how rails does it: http://guides.rubyonrails.org/layouts_and_rendering.html#using-partials (see section 3.4.4).

@@ -186,7 +186,8 @@ def add_all_cmd_tlm(dir)
if Dir.exist?(File.join(dir, 'cmd_tlm'))
# Only grab *.txt files in the root of the cmd_tlm folder
Dir[File.join(dir, 'cmd_tlm', '*.txt')].each do |filename|
cmd_tlm_files << filename
# Don't add partial files which begin with '_'
cmd_tlm_files << filename unless filename[0] == '_'
Copy link

Choose a reason for hiding this comment

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

The partials need to stay in the cmd_tlm_files list so that they will be included in the MD5 calculation. You just need to modify System#load_packets to not call process_file on them. Not sure if that should be based on .erb extension. See higher level comment

Copy link

Choose a reason for hiding this comment

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

Probably should be based on leading underscore

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, I was recreating the marshal file each time. I ended up letting packet_config.rb decide whether to process the file.

@ghost
Copy link

ghost commented Jul 16, 2015

This is an awesome feature! Well done putting it together so fast

@jmthomas
Copy link
Contributor Author

I added a warning about deprecating MACRO_APPEND_START/STOP because it's totally not needed now that we have ERB template support.

@ghost
Copy link

ghost commented Jul 17, 2015

I fixed an issue where if you where manually specifying the COMMANDS and TELEMETRY in the target.txt files that partials weren't getting added to the file list. I also made it support subfolders. Finally, I removed the parsed_ debugging files as they were being written into areas that I don't want COSMOS touching and I don't want the config_parser.rb file to be dependent on System to look up the tmp path, etc.

Approved. It is also just hit me that this is now available in ALL config files which is huge.

@jmthomas
Copy link
Contributor Author

All you did was delete the code that writes out the debug files. They were being written right back into the same dir that they originated from. Now if you have an error in your partial you can't see the final result and it's impossible to debug.

@ghost
Copy link

ghost commented Jul 17, 2015

It's not impossible, you still get the exception. There are lots of problems with writing into the target folders.

  1. I consider it to be bad form to write into the target folders as they could be read-only
  2. You now silently have two config files that will get parsed next time you startup

If we keep the functionality I would like it to write to the tmp folder. I guess we could make it try to write to the default tmp folder if it exists, but I don't want config_parser.rb to become dependent on System. Let me update it to do that.

@ghost
Copy link

ghost commented Jul 17, 2015

Also, the parsed_ file may not have any ERB and may be an exact copy of the given config file. These files get created even for normal configuration errors.

@jmthomas
Copy link
Contributor Author

Many good points. Perhaps we just could popup a dialog with the parsed output? That's kind of messy, especially for a long config file. I guess I don't care where the parsed output goes as long as the user can get to it for debugging purposes.

@ghost
Copy link

ghost commented Jul 17, 2015

Added an updated working error handler back. Check it out.

… the tlm_extractor config files. Use ERB in Table Manager config.
@jmthomas
Copy link
Contributor Author

I want that output so I made it always output using the root USERPATH if necessary. Also I added the output filename to the exception message so people know where to go look. Also updated a few config files to use ERB. Why didn't we do this sooner?!?

@ghost
Copy link

ghost commented Jul 17, 2015

Did a little more cleanup. All specs pass. This is approved.

jmthomas added a commit that referenced this pull request Jul 17, 2015
@jmthomas jmthomas merged commit e7d098e into master Jul 17, 2015
@jmthomas jmthomas deleted the support_cmd_tlm_partials branch July 17, 2015 19:44
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 88.698% when pulling 936dfd4 on support_cmd_tlm_partials into 9042623 on master.

ghost pushed a commit that referenced this pull request Aug 24, 2021
Merge in COSMOSEE/base from alpine-container-update to master

Squashed commit of the following:

commit ce64e641f64231cee3455d1356c28c086e95049c
Author: van Andel, Gerhard <[email protected]>
Date:   Tue Aug 24 14:24:07 2021 -0600

    use alpine 3.14.1
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